PARQUET-2249: Write IEEE 754 total order by default for floating-point columns - #3699
PARQUET-2249: Write IEEE 754 total order by default for floating-point columns#3699Jiayi-Wang-db wants to merge 3 commits into
Conversation
9388a3e to
38f5429
Compare
…t columns Follow-up to apache#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 apache#3393. This makes FLOAT, DOUBLE and FLOAT16 columns built without an explicit column order default to IEEE_754_TOTAL_ORDER (mirroring how apache#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
38f5429 to
c8a7179
Compare
|
@Fokko @gszadovszky Do you want to take a look? |
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
Based on the dev-list discussion thread, this makes sense to me 👍
| throw new IllegalArgumentException( | ||
| "Unsupported column order: " + name + " at " + st.getLocationString()); |
There was a problem hiding this comment.
Should we throw here, or emit a null/unknown ColumnOrder?
There was a problem hiding this comment.
Good point, I think it should be degraded to Undefined, and not throw explicit exception. Made the change.
Address review feedback: MessageTypeParser.parseColumnOrder now returns
ColumnOrder.undefined() for a columnorder(...) token it does not recognize,
instead of throwing. This matches ParquetMetadataConverter.fromParquetColumnOrder
("not yet supported by this API") so a schema string written by a newer API with
an order this version does not know stays parseable; statistics under an unknown
order are ignored by readers anyway. UNDEFINED is a valid order for all primitive
types, so it is safe to feed into the builder.
Co-authored-by: Isaac
|
It looks good to me. There is however a potential follow up issue from codex:
|
Address review follow-up (codex via @gszadovszky): with floats defaulting to IEEE_754_TOTAL_ORDER and legacy footers read as TYPE_DEFINED_ORDER, aggregating footers over a directory that spans the upgrade (e.g. ParquetInputFormat split planning / getGlobalMetaData, or the deprecated summary-file merge) threw IncompatibleSchemaModificationException on the otherwise-identical float column. PrimitiveType.union now reconciles a column-order-only difference to UNDEFINED instead of failing. At that point type, logical type and length already match, so the order is the only difference and the columns are otherwise mergeable. This is safe because per-file statistics are still read under each file's own column order (from its own footer); only the merged schema's ambiguous ordering claim is dropped. Added TestMessageType.testMergeMixedFloatingColumnOrder and updated testMergeSchemaWithColumnOrder, which previously asserted the merge threw. Co-authored-by: Isaac
Hi Gobor, thanks for flagging this. It makes sense to me that we should use the |
4 participants
Rationale for this change
Under the new writer behavior, a floating-point column containing NaN gets finite min/max computed over the non-NaN values, together with a
nan_count. A reader that predatesnan_countignores it, accepts the finite bounds, and can incorrectly prune row groups that actually contain NaN. e.g. [1.0, NaN] is written as min = max = 1.0, so an old reader drops the row group for greater(x, 1.0) even though NaN matches.By contrast, readers ignore statistics written under an unknown sort order. Writing IEEE_754_TOTAL_ORDER by default is therefore the safer, forward-compatible behavior: older readers simply decline to use the stats they can't interpret rather than misinterpreting them.
What changes are included in this PR?
FLOAT, DOUBLE and FLOAT16 columns built without an explicit column order now default to
IEEE_754_TOTAL_ORDERAre these changes tested?
yes
Are there any user-facing changes?
Yes. Floating-point columns are now written with IEEE_754_TOTAL_ORDER by default instead of TYPE_DEFINED_ORDER.