Skip to content

Commit c8a7179

Browse files
Jiayi-Wang-dbJiayi-Wang-db
committed
PARQUET-2249: Write IEEE 754 total order by default for floating-point columns
Follow-up to #3393, which added IEEE_754_TOTAL_ORDER support but kept TYPE_DEFINED_ORDER as the default for FLOAT, DOUBLE and FLOAT16 columns. Keeping the type-defined order as the default is a latent backward-compat hazard: the writer now computes finite min/max over the non-NaN subset and records nan_count, but an old reader that predates nan_count ignores it, accepts the finite bounds, and can incorrectly prune row groups that contain NaN. Readers instead ignore statistics written under an unknown sort order, so writing IEEE 754 total order by default is the safer behavior. See the discussion on #3393. This makes FLOAT, DOUBLE and FLOAT16 columns built without an explicit column order default to IEEE_754_TOTAL_ORDER (mirroring how #3610 defaults INT96 to INT96_TIMESTAMP_ORDER). Columns with a logical annotation that does not accept IEEE 754 total order (e.g. an unknown annotation) fall back to type-defined order so they remain constructible. The default-order selection is unified in PrimitiveType.defaultColumnOrder so construction and text serialization agree. To stay backward compatible on read, a footer that carries no column_orders list predates IEEE_754_TOTAL_ORDER, so floating-point columns read from such a footer are given type-defined order rather than inheriting the new construction-time default; their legacy statistics are thus not reinterpreted under IEEE 754 total order. The text schema representation now carries a non-default column order (columnorder(...) after the type/annotation) and MessageTypeParser parses it, so a column order set explicitly survives toString()/parse round-trips such as the one GroupWriteSupport performs. Columns left at their default emit no token, keeping existing schema strings unchanged. Tests that exercise the legacy type-defined NaN / +-0 semantics set TYPE_DEFINED_ORDER explicitly, and new tests cover the default serialization, the column-order-less read path, and the text round-trip. Co-authored-by: Isaac
1 parent 2c9fbb3 commit c8a7179

14 files changed

Lines changed: 211 additions & 26 deletions

File tree

‎parquet-column/src/main/java/org/apache/parquet/schema/MessageTypeParser.java‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -225,6 +225,12 @@ private static void addPrimitiveType(
225225
check(t, ")", "logical type ended by )", st);
226226
t = st.nextToken();
227227
}
228+
if (t.equalsIgnoreCase(PrimitiveType.COLUMN_ORDER_KEYWORD)) {
229+
check(st.nextToken(), "(", "column order followed by (", st);
230+
childBuilder.columnOrder(parseColumnOrder(st.nextToken(), st));
231+
check(st.nextToken(), ")", "column order ended by )", st);
232+
t = st.nextToken();
233+
}
228234
if (t.equals("=")) {
229235
childBuilder.id(Integer.parseInt(st.nextToken()));
230236
t = st.nextToken();
@@ -240,6 +246,26 @@ private static void addPrimitiveType(
240246
}
241247
}
242248

249+
private static ColumnOrder parseColumnOrder(String t, Tokenizer st) {
250+
ColumnOrder.ColumnOrderName name;
251+
try {
252+
name = ColumnOrder.ColumnOrderName.valueOf(t.toUpperCase(Locale.ENGLISH));
253+
} catch (IllegalArgumentException e) {
254+
throw new IllegalArgumentException("Unknown column order: " + t + " at " + st.getLocationString(), e);
255+
}
256+
switch (name) {
257+
case UNDEFINED:
258+
return ColumnOrder.undefined();
259+
case TYPE_DEFINED_ORDER:
260+
return ColumnOrder.typeDefined();
261+
case IEEE_754_TOTAL_ORDER:
262+
return ColumnOrder.ieee754TotalOrder();
263+
default:
264+
throw new IllegalArgumentException(
265+
"Unsupported column order: " + name + " at " + st.getLocationString());
266+
}
267+
}
268+
243269
private static boolean isLogicalType(String t) {
244270
return Arrays.stream(LogicalTypeAnnotation.LogicalTypeToken.values())
245271
.anyMatch((type) -> type.name().equals(t));

‎parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveType.java‎

Lines changed: 33 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -492,6 +492,9 @@ public PrimitiveComparator<?> comparator(LogicalTypeAnnotation logicalType) {
492492
}
493493
}
494494

495+
// Keyword used to render/parse a non-default column order in the text schema representation.
496+
static final String COLUMN_ORDER_KEYWORD = "columnorder";
497+
495498
private final PrimitiveTypeName primitive;
496499
private final int length;
497500
private final DecimalMetadata decimalMeta;
@@ -578,9 +581,7 @@ public PrimitiveType(
578581
this.decimalMeta = decimalMeta;
579582

580583
if (columnOrder == null) {
581-
columnOrder = primitive == PrimitiveTypeName.INT96 || originalType == OriginalType.INTERVAL
582-
? ColumnOrder.undefined()
583-
: ColumnOrder.typeDefined();
584+
columnOrder = defaultColumnOrder(primitive, originalType, getLogicalTypeAnnotation());
584585
} else if (columnOrder.getColumnOrderName() == ColumnOrderName.IEEE_754_TOTAL_ORDER) {
585586
Preconditions.checkArgument(
586587
primitive == PrimitiveTypeName.FLOAT || primitive == PrimitiveTypeName.DOUBLE,
@@ -629,10 +630,7 @@ public PrimitiveType(
629630
}
630631

631632
if (columnOrder == null) {
632-
columnOrder = primitive == PrimitiveTypeName.INT96
633-
|| logicalTypeAnnotation instanceof LogicalTypeAnnotation.IntervalLogicalTypeAnnotation
634-
? ColumnOrder.undefined()
635-
: ColumnOrder.typeDefined();
633+
columnOrder = defaultColumnOrder(primitive, getOriginalType(), logicalTypeAnnotation);
636634
} else if (columnOrder.getColumnOrderName() == ColumnOrderName.IEEE_754_TOTAL_ORDER) {
637635
Preconditions.checkArgument(
638636
primitive == PrimitiveTypeName.FLOAT
@@ -648,6 +646,27 @@ public PrimitiveType(
648646
this.columnOrder = requireValidColumnOrder(columnOrder);
649647
}
650648

649+
/**
650+
* The column order used when none is specified explicitly. INT96 and INTERVAL have no defined
651+
* ordering, so they default to undefined. Floating-point types default to IEEE 754 total order so
652+
* that NaN values and the sign of zero are ordered deterministically and nan_count statistics can
653+
* be written; this is skipped when the logical type annotation does not accept IEEE 754 total
654+
* order (e.g. an unknown annotation), leaving the type constructible with the type-defined order.
655+
*/
656+
private static ColumnOrder defaultColumnOrder(
657+
PrimitiveTypeName primitive, OriginalType originalType, LogicalTypeAnnotation logicalTypeAnnotation) {
658+
if (primitive == PrimitiveTypeName.INT96 || originalType == OriginalType.INTERVAL) {
659+
return ColumnOrder.undefined();
660+
}
661+
boolean isFloatingType = primitive == PrimitiveTypeName.FLOAT
662+
|| primitive == PrimitiveTypeName.DOUBLE
663+
|| (logicalTypeAnnotation != null
664+
&& logicalTypeAnnotation.getType() == LogicalTypeAnnotation.LogicalTypeToken.FLOAT16);
665+
boolean acceptsIeee754 = logicalTypeAnnotation == null
666+
|| logicalTypeAnnotation.isValidColumnOrder(ColumnOrder.ieee754TotalOrder());
667+
return isFloatingType && acceptsIeee754 ? ColumnOrder.ieee754TotalOrder() : ColumnOrder.typeDefined();
668+
}
669+
651670
private ColumnOrder requireValidColumnOrder(ColumnOrder columnOrder) {
652671
if (primitive == PrimitiveTypeName.INT96) {
653672
Preconditions.checkArgument(
@@ -748,6 +767,13 @@ public void writeToStringBuilder(StringBuilder sb, String indent) {
748767
// TODO: should we print decimal metadata too?
749768
sb.append(" (").append(getLogicalTypeAnnotation().toString()).append(")");
750769
}
770+
// Only emit the column order when it differs from the default, so schemas that rely on the
771+
// default stay textually unchanged.
772+
if (!columnOrder.equals(defaultColumnOrder(primitive, getOriginalType(), getLogicalTypeAnnotation()))) {
773+
sb.append(" ").append(COLUMN_ORDER_KEYWORD).append("(");
774+
sb.append(columnOrder.getColumnOrderName().name());
775+
sb.append(")");
776+
}
751777
if (getId() != null) {
752778
sb.append(" = ").append(getId());
753779
}

‎parquet-column/src/main/java/org/apache/parquet/schema/Types.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -414,9 +414,11 @@ public THIS scale(int scale) {
414414
/**
415415
* Adds the column order for the primitive type.
416416
* <p>
417-
* In case of not set the default column order is {@link ColumnOrderName#TYPE_DEFINED_ORDER} except the type
418-
* {@link PrimitiveTypeName#INT96} and the types annotated by {@link OriginalType#INTERVAL} where the default column
419-
* order is {@link ColumnOrderName#UNDEFINED}.
417+
* In case of not set the default column order is {@link ColumnOrderName#TYPE_DEFINED_ORDER}, with the following
418+
* exceptions: the floating-point types {@link PrimitiveTypeName#FLOAT}, {@link PrimitiveTypeName#DOUBLE} and the
419+
* {@code FLOAT16} logical type default to {@link ColumnOrderName#IEEE_754_TOTAL_ORDER}; the type
420+
* {@link PrimitiveTypeName#INT96} and the types annotated by {@link OriginalType#INTERVAL} default to
421+
* {@link ColumnOrderName#UNDEFINED}.
420422
*
421423
* @param columnOrder the column order for the primitive type
422424
* @return this builder for method chaining

‎parquet-column/src/test/java/org/apache/parquet/column/statistics/TestStatistics.java‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
import java.nio.ByteBuffer;
3939
import java.util.Locale;
4040
import org.apache.parquet.io.api.Binary;
41+
import org.apache.parquet.schema.ColumnOrder;
4142
import org.apache.parquet.schema.OriginalType;
4243
import org.apache.parquet.schema.PrimitiveType;
4344
import org.apache.parquet.schema.PrimitiveType.PrimitiveTypeName;
@@ -775,7 +776,8 @@ private void testBuilder(PrimitiveType type, Object min, byte[] minBytes, Object
775776

776777
@Test
777778
public void testSpecBuilderForFloat() {
778-
PrimitiveType type = Types.required(FLOAT).named("test_float");
779+
PrimitiveType type =
780+
Types.required(FLOAT).columnOrder(ColumnOrder.typeDefined()).named("test_float");
779781
Statistics.Builder builder = Statistics.getBuilderForReading(type);
780782
Statistics<?> stats = builder.withMin(intToBytes(floatToIntBits(Float.NaN)))
781783
.withMax(intToBytes(floatToIntBits(42.0f)))
@@ -839,7 +841,8 @@ public void testSpecBuilderForFloat() {
839841

840842
@Test
841843
public void testSpecBuilderForDouble() {
842-
PrimitiveType type = Types.required(DOUBLE).named("test_double");
844+
PrimitiveType type =
845+
Types.required(DOUBLE).columnOrder(ColumnOrder.typeDefined()).named("test_double");
843846
Statistics.Builder builder = Statistics.getBuilderForReading(type);
844847
Statistics<?> stats = builder.withMin(longToBytes(doubleToLongBits(Double.NaN)))
845848
.withMax(longToBytes(doubleToLongBits(42.0)))

‎parquet-column/src/test/java/org/apache/parquet/column/statistics/TestStatisticsNanCount.java‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,13 +32,16 @@
3232

3333
public class TestStatisticsNanCount {
3434

35-
private static final PrimitiveType FLOAT_TYPE =
36-
Types.optional(PrimitiveTypeName.FLOAT).named("test_float");
37-
private static final PrimitiveType DOUBLE_TYPE =
38-
Types.optional(PrimitiveTypeName.DOUBLE).named("test_double");
35+
private static final PrimitiveType FLOAT_TYPE = Types.optional(PrimitiveTypeName.FLOAT)
36+
.columnOrder(ColumnOrder.typeDefined())
37+
.named("test_float");
38+
private static final PrimitiveType DOUBLE_TYPE = Types.optional(PrimitiveTypeName.DOUBLE)
39+
.columnOrder(ColumnOrder.typeDefined())
40+
.named("test_double");
3941
private static final PrimitiveType FLOAT16_TYPE = Types.optional(PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY)
4042
.length(2)
4143
.as(LogicalTypeAnnotation.float16Type())
44+
.columnOrder(ColumnOrder.typeDefined())
4245
.named("test_float16");
4346

4447
private static final PrimitiveType FLOAT_IEEE754_TYPE = Types.optional(PrimitiveTypeName.FLOAT)

‎parquet-column/src/test/java/org/apache/parquet/internal/column/columnindex/TestColumnIndexBuilder.java‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@
6969
import org.apache.parquet.filter2.predicate.Operators.LongColumn;
7070
import org.apache.parquet.filter2.predicate.UserDefinedPredicate;
7171
import org.apache.parquet.io.api.Binary;
72+
import org.apache.parquet.schema.ColumnOrder;
7273
import org.apache.parquet.schema.PrimitiveType;
7374
import org.apache.parquet.schema.Types;
7475
import org.junit.jupiter.api.Test;
@@ -1030,7 +1031,8 @@ public void testBuildDouble() {
10301031

10311032
@Test
10321033
public void testBuildDoubleZeroNaN() {
1033-
PrimitiveType type = Types.required(DOUBLE).named("test_double");
1034+
PrimitiveType type =
1035+
Types.required(DOUBLE).columnOrder(ColumnOrder.typeDefined()).named("test_double");
10341036
ColumnIndexBuilder builder = ColumnIndexBuilder.getBuilder(type, Integer.MAX_VALUE);
10351037
StatsBuilder sb = new StatsBuilder();
10361038
builder.add(sb.stats(type, -1.0, -0.0));
@@ -1183,7 +1185,8 @@ public void testBuildFloat() {
11831185

11841186
@Test
11851187
public void testBuildFloatZeroNaN() {
1186-
PrimitiveType type = Types.required(FLOAT).named("test_float");
1188+
PrimitiveType type =
1189+
Types.required(FLOAT).columnOrder(ColumnOrder.typeDefined()).named("test_float");
11871190
ColumnIndexBuilder builder = ColumnIndexBuilder.getBuilder(type, Integer.MAX_VALUE);
11881191
StatsBuilder sb = new StatsBuilder();
11891192
builder.add(sb.stats(type, -1.0f, -0.0f));

‎parquet-column/src/test/java/org/apache/parquet/internal/column/columnindex/TestColumnIndexBuilderNaN.java‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -44,19 +44,22 @@
4444
*/
4545
public class TestColumnIndexBuilderNaN {
4646

47-
private static final PrimitiveType FLOAT_TYPE =
48-
Types.required(PrimitiveTypeName.FLOAT).named("test_float");
47+
private static final PrimitiveType FLOAT_TYPE = Types.required(PrimitiveTypeName.FLOAT)
48+
.columnOrder(ColumnOrder.typeDefined())
49+
.named("test_float");
4950
private static final PrimitiveType FLOAT_IEEE754_TYPE = Types.required(PrimitiveTypeName.FLOAT)
5051
.columnOrder(ColumnOrder.ieee754TotalOrder())
5152
.named("test_float_ieee754");
52-
private static final PrimitiveType DOUBLE_TYPE =
53-
Types.required(PrimitiveTypeName.DOUBLE).named("test_double");
53+
private static final PrimitiveType DOUBLE_TYPE = Types.required(PrimitiveTypeName.DOUBLE)
54+
.columnOrder(ColumnOrder.typeDefined())
55+
.named("test_double");
5456
private static final PrimitiveType DOUBLE_IEEE754_TYPE = Types.required(PrimitiveTypeName.DOUBLE)
5557
.columnOrder(ColumnOrder.ieee754TotalOrder())
5658
.named("test_double_ieee754");
5759
private static final PrimitiveType FLOAT16_TYPE = Types.required(PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY)
5860
.length(2)
5961
.as(LogicalTypeAnnotation.float16Type())
62+
.columnOrder(ColumnOrder.typeDefined())
6063
.named("test_float16");
6164
private static final PrimitiveType FLOAT16_IEEE754_TYPE = Types.required(PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY)
6265
.length(2)

‎parquet-column/src/test/java/org/apache/parquet/schema/TestMessageType.java‎

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,37 @@ public void testMergeSchemaWithColumnOrder() {
202202
assertThatThrownBy(() -> m1.union(m3))
203203
.isInstanceOf(IncompatibleSchemaModificationException.class)
204204
.hasMessage(
205-
"can not merge type optional binary a with column order TYPE_DEFINED_ORDER into optional binary a with column order UNDEFINED");
205+
"can not merge type optional binary a with column order TYPE_DEFINED_ORDER into optional binary a columnorder(UNDEFINED) with column order UNDEFINED");
206+
}
207+
208+
@Test
209+
public void testColumnOrderTextRoundTrip() {
210+
// A non-default column order must survive toString() -> parseMessageType() so that schemas
211+
// serialized through the text representation (e.g. by GroupWriteSupport) keep it.
212+
MessageType schema = Types.buildMessage()
213+
.required(PrimitiveTypeName.FLOAT)
214+
.columnOrder(ColumnOrder.typeDefined())
215+
.named("float_typedef")
216+
.required(PrimitiveTypeName.DOUBLE)
217+
.columnOrder(ColumnOrder.ieee754TotalOrder())
218+
.named("double_ieee754")
219+
.required(PrimitiveTypeName.INT32)
220+
.named("int_default")
221+
.named("msg");
222+
223+
assertThat(schema.getType("float_typedef").asPrimitiveType().columnOrder())
224+
.isEqualTo(ColumnOrder.typeDefined());
225+
MessageType roundTripped = MessageTypeParser.parseMessageType(schema.toString());
226+
assertThat(roundTripped).isEqualTo(schema);
227+
assertThat(roundTripped.getType("float_typedef").asPrimitiveType().columnOrder())
228+
.isEqualTo(ColumnOrder.typeDefined());
229+
assertThat(roundTripped.getType("double_ieee754").asPrimitiveType().columnOrder())
230+
.isEqualTo(ColumnOrder.ieee754TotalOrder());
231+
assertThat(roundTripped.getType("int_default").asPrimitiveType().columnOrder())
232+
.isEqualTo(ColumnOrder.typeDefined());
233+
234+
// A column left at its default emits no columnorder(...) token.
235+
assertThat(schema.toString()).doesNotContain("int_default columnorder");
206236
}
207237

208238
@Test

‎parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2061,6 +2061,13 @@ private void buildChildren(
20612061
columnOrder = org.apache.parquet.schema.ColumnOrder.undefined();
20622062
}
20632063
primitiveBuilder.columnOrder(columnOrder);
2064+
} else if (schemaElement.type == Type.FLOAT
2065+
|| schemaElement.type == Type.DOUBLE
2066+
|| (schemaElement.isSetLogicalType() && schemaElement.logicalType.isSetFLOAT16())) {
2067+
// A footer without column orders predates IEEE_754_TOTAL_ORDER, so a floating-point column
2068+
// here must not inherit the (IEEE 754 total order) construction-time default: its stats, if
2069+
// any, were written under the legacy type-defined order and must be read under it.
2070+
primitiveBuilder.columnOrder(org.apache.parquet.schema.ColumnOrder.typeDefined());
20642071
}
20652072
childBuilder = primitiveBuilder;
20662073
} else {

‎parquet-hadoop/src/test/java/org/apache/parquet/filter2/dictionarylevel/DictionaryFilterTest.java‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -706,15 +706,20 @@ private static List<ColumnChunkMetaData> nanColumns() {
706706
return List.of(
707707
nanColumn(
708708
"double_nan_field",
709-
Types.required(PrimitiveTypeName.DOUBLE).named("double_nan_field")),
709+
Types.required(PrimitiveTypeName.DOUBLE)
710+
.columnOrder(ColumnOrder.typeDefined())
711+
.named("double_nan_field")),
710712
nanColumn(
711713
"float_nan_field",
712-
Types.required(PrimitiveTypeName.FLOAT).named("float_nan_field")),
714+
Types.required(PrimitiveTypeName.FLOAT)
715+
.columnOrder(ColumnOrder.typeDefined())
716+
.named("float_nan_field")),
713717
nanColumn(
714718
"float16_nan_field",
715719
Types.required(PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY)
716720
.length(2)
717721
.as(LogicalTypeAnnotation.float16Type())
722+
.columnOrder(ColumnOrder.typeDefined())
718723
.named("float16_nan_field")));
719724
}
720725

0 commit comments

Comments
 (0)