ORC: Apply top-level scalar initial-default on read (absent-only fill)#252
ORC: Apply top-level scalar initial-default on read (absent-only fill)#252cbb330 wants to merge 6 commits into
Conversation
Fill a field's initial-default as a per-file constant when its column is absent from an ORC data file, for the generic, Spark (2.4/3.1/3.2/3.3, row + vectorized) and Flink (1.14/1.15/1.16) readers. buildOrcProjection omits an absent top-level scalar field that declares an initial-default; each reader's record() step then injects the converted default into idToConstant so the existing partition-constant path fills it for every row (consuming no column vector). The omit is gated on applyDefaults, which OrcIterable sets to hasIds(fileSchema): id-less (name-mapped/migrated) files synthesize a null column instead and read NULL, so a default is never name-matched onto a file lacking embedded field ids. Scope is top-level scalar defaults; nested defaults are synthesized as null (read NULL) and remain a follow-up. Present-column binding is unchanged (ORC name-evolution + positional assembly), so existing reads do not regress. Co-authored-by: Cursor <cursoragent@cursor.com>
cbb330
left a comment
There was a problem hiding this comment.
Inline provenance annotations as requested, so the split is visible per-hunk.
Legend: 🟣 PRIOR ART (LI fork #76, same positional reader generation) · 🔵 BACKPORTED (decision/semantics carried from my apache/main ORC PR, re-implemented for the positional reader) · 🟢 NET NEW (did not exist on main or in #76).
Mental model: #76 = the how (omit + idToConstant inject on a positional reader); main = the what/why (top-level scalar scope, hasIds gate, NULL fallbacks); the shared idToConstantWithDefaults helper + all-engine wiring = the net-new glue.
cbb330
left a comment
There was a problem hiding this comment.
Stamping the remaining files so no file is left to interpretation. These are the per-version copies of the spots already annotated on the v3.3 / v1.16 representatives, plus the tests. Same legend: 🟣 PRIOR ART (#76) · 🔵 BACKPORTED (from apache/main) · 🟢 NET NEW.
Add an opt-in applyColumnDefaults flag on the ORC read builder (off by default). Only the row SparkOrcReader (Spark 3.1) and generic GenericOrcReader read paths enable it; OrcIterable then omits an absent top-level scalar default only when opted in (and the file has ids), so the reader can inject it via idToConstant. Every other ORC reader leaves the flag off, keeping the column synthesized as a null column (reads NULL) and therefore positionally aligned -- so it cannot misalign. Defers vectorized ORC and Flink defaults to a follow-up, and drops the Spark 2.4/3.2/3.3 wiring (those versions read NULL, unchanged). Co-authored-by: Cursor <cursoragent@cursor.com>
cbb330
left a comment
There was a problem hiding this comment.
Re-stamped provenance for the now-scoped file set (generic + Spark 3.1 row + the opt-in gate). Supersedes my earlier two reviews, which were partly anchored to files that have since been reverted.
Legend: 🟣 PRIOR ART (LI fork #76, positional reader) · 🔵 BACKPORTED (decision/semantics from my apache/main ORC PR) · 🟢 NET NEW (did not exist on main or #76 — includes the opt-in gate introduced in this revision).
…th-default Add typed-default cases (boolean/int/long/float/double/string/decimal) and a required-field-with-default case (end-to-end + projection-level), confirming the fill reuses the partition-constant channel correctly across scalar types and that an absent required field with a default is omitted+filled rather than rejected by the required-missing check. Co-authored-by: Cursor <cursoragent@cursor.com>
Drop the top-level-only gate in buildOrcProjection so an absent scalar field declaring an initial-default is omitted (and filled via idToConstant) at any nesting level, not just the top level. The reader path already keys idToConstant by global field id and consults it at every struct level, so no reader change is needed. Add a safeguard so a non-root struct is never emitted empty (which ORC can reject): if every subfield of a nested struct is absent + defaulted, keep one synthesized null column (it reads NULL). The root struct may still be empty (select-only-default). Nested fill scenarios (struct/map/list) are ported from upstream BaseFormatModelTests. Co-authored-by: Cursor <cursoragent@cursor.com>
…ed structs) ORC accepts an empty nested read struct (as it already does for the empty root struct in select-only-default), so the "keep one synthesized null column" safeguard was unnecessary and introduced a surprising case where one subfield read NULL instead of its default. Remove it: an absent scalar default is now omitted and filled at any nesting level with no exceptions, including when every subfield of a nested struct is absent + defaulted (covered by a new end-to-end test). Also drops the now-unused topLevel parameter from buildOrcProjection. Co-authored-by: Cursor <cursoragent@cursor.com>
Add a row with a null nested struct to testNestedStructScalarDefault: a null parent struct stays null on read (the absent-only default is not fabricated), locking the positional reader's null-struct branch. Upstream's nested-default tests only cover this incidentally via random data, and via the id-binding reader rather than this branch. Co-authored-by: Cursor <cursoragent@cursor.com>
cbb330
left a comment
There was a problem hiding this comment.
Provenance re-stamp for the current state (now includes nested-scalar defaults + the opt-in gate). Supersedes earlier reviews.
Legend: 🟣 PRIOR ART (LI fork #76, positional reader) · 🔵 BACKPORTED (decision/semantics from my apache/main ORC PR) · 🟢 NET NEW (did not exist on main or #76 — chiefly the opt-in gate and the single shared helper).
| * so the reader can inject it; when disabled, the field is synthesized as a null column and | ||
| * reads NULL. The omit is additionally gated on the file carrying embedded field ids. | ||
| */ | ||
| public ReadBuilder applyColumnDefaults(boolean newApplyColumnDefaults) { |
There was a problem hiding this comment.
🟢 NET NEW — the opt-in gate. Default-application is OFF unless a read calls applyColumnDefaults(true). Neither main nor #76 had this; it keeps the feature contained so every un-wired ORC reader (vectorized, Flink, Spark 2.4/3.2/3.3, MR) reads NULL and stays positionally aligned.
| // read | ||
| // paths). Other readers leave it false, so an absent top-level scalar default is synthesized | ||
| // as a null column (reads NULL) rather than omitted, which keeps those readers aligned. | ||
| readOrcSchema = ORCSchemaUtil.buildOrcProjection(schema, fileSchema, applyColumnDefaults); |
There was a problem hiding this comment.
🟢 NET NEW + 🔵 BACKPORTED — applyDefaults = applyColumnDefaults (opt-in, 🟢) on the id path; id-less/name-mapped stays false. The hasIds gating is 🔵 from main; the opt-in flag is the net-new piece.
| * @param convertConstant converts an internal default value to the engine's in-memory form | ||
| * @return {@code idToConstant} unchanged when no defaults apply, otherwise a new merged map | ||
| */ | ||
| public static Map<Integer, ?> idToConstantWithDefaults( |
There was a problem hiding this comment.
🟢 NET NEW — shared, engine-agnostic default-injection helper. main fills inside the id-binding StructReader; #76 injected inline in a Spark-only visitor. One helper keyed on "expected field absent from the ORC record", reused by the generic + Spark-3.1-row record() steps at every nesting level. The BiFunction is the only per-engine seam.
| Integer.MIN_VALUE, schema.asStruct(), true, applyDefaults, icebergToOrc); | ||
| } | ||
|
|
||
| private static boolean isOmittableDefault( |
There was a problem hiding this comment.
🔵 BACKPORTED (from main) — omit an absent scalar field that declares a default from the read projection, at ANY nesting level (matches main's scope). The reader then fills it via idToConstant. Empty nested read-structs are fine (verified against ORC source + an end-to-end test), so no special-casing.
| return GenericOrcReaders.struct( | ||
| fields, | ||
| expected, | ||
| ORCSchemaUtil.idToConstantWithDefaults( |
There was a problem hiding this comment.
🟢 NET NEW (breadth) — generic reader wiring via the shared helper + IdentityPartitionConverters::convertConstant. #76 was Spark-only. Active only when the generic read opts in (see GenericReader).
| } | ||
|
|
||
| @Test | ||
| public void testNestedStructScalarDefault() throws IOException { |
There was a problem hiding this comment.
🔵 BACKPORTED — end-to-end fill tests; the nested struct/map-value/list-element scenarios are ported from upstream BaseFormatModelTests (JUnit4). Also covers all scalar types, required-with-default, empty nested struct, and a null nested struct (default not fabricated for a null parent).
| return SparkOrcValueReaders.struct( | ||
| fields, | ||
| expected, | ||
| ORCSchemaUtil.idToConstantWithDefaults( |
There was a problem hiding this comment.
| } | ||
|
|
||
| protected static Object convertConstant(Type type, Object value) { | ||
| public static Object convertConstant(Type type, Object value) { |
| .filter(task.residual()) | ||
| .caseSensitive(caseSensitive); | ||
| .caseSensitive(caseSensitive) | ||
| .applyColumnDefaults(true); |
There was a problem hiding this comment.
🟢 NET NEW — Spark 3.1 row read opts into the gate. The vectorized path deliberately does NOT opt in (reads NULL) — vectorized is the follow-up. Inherited by EqualityDeleteRowReader and RowDataRewriter (compaction).
| } | ||
|
|
||
| @Test | ||
| public void testRowReadReadsNullWhenNotOptedIn() throws IOException { |
There was a problem hiding this comment.
🔵 BACKPORTED + gate proof — mirrors main's TestSparkOrcReaderDefaults (row fill), plus a net-new assertion that without opt-in the same read returns NULL, proving the gate is off by default.
Summary
Fills a scalar column initial-default on ORC read when the column is absent from the data file — at any nesting level (top-level fields and scalar subfields of structs/maps/lists) — for the generic (
GenericOrcReader) and Spark 3.1 row (SparkOrcReader) read paths. Builds on the IC-1 API already on this branch (#250).Mechanism (the #76 positional pattern, retargeted to
initialDefault)ORCSchemaUtil.buildOrcProjection(..., applyDefaults)omits an absent scalar field that declares aninitial-default(at any level); the reader'srecord()step injects the converted default intoidToConstant(ORCSchemaUtil.idToConstantWithDefaults), and the existing positional partition-constant path fills it for every row, consuming no column vector. The hot read loop is unchanged.idToConstantis keyed by global field id and consulted at every struct level, so nested fill needs no reader change.Opt-in gate (off by default)
ORC.read()...applyColumnDefaults(boolean)flag, off by default.OrcIterableomits defaulted columns only when a read opts in and the file carries embedded ids.RowDataReader, inherited byEqualityDeleteRowReaderandRowDataRewriter/compaction) and the generic path (GenericReader) opt in. Every other ORC reader (vectorized everywhere, Flink, Spark 2.4/3.2/3.3, MR) leaves the flag off, so columns are synthesized as null and read NULL — they stay positionally aligned and cannot misalign.initial-defaultis never name-matched onto a legacy/migrated file.No regression
Present-column binding is unchanged (ORC name-evolution + positional assembly). With the gate off by default and the fill guarded by
initialDefault() != null, existing reads are byte-for-byte unaffected.Deferred / out of scope
Where a default isn't applied, the column reads NULL (no wrong data silently returned), except the filter case noted below.
SELECTof a defaulted column is correct, but a pushed predicate on it (e.g.WHERE country = 'US') can error or mis-prune, because the column is omitted from the ORC SearchArgument schema. Avoid filtering on defaulted columns until then.Literal-onlyinitialDefaultAPI, on any branch.writeDefault(value written when a new write omits the column) is not handled;initial-defaultis never persisted tometadata.json(by design).Scope (in this PR)
Generic + Spark 3.1 row, scalar defaults at any nesting level, gated on embedded field ids.
Test plan
orcTestBuildOrcProjection— omit whenapplyDefaults=true(top-level + nested); synthesize/read-NULL when false; required-with-default omitted (not rejected); nested struct emptied by omit.orcTestOrcDefaultValues— generic round-trip fills the default; select-only-default (empty projection) reads the default; all scalar types (boolean/int/long/float/double/string/decimal); required-field-with-default; nested struct / map-value / list-element scalar defaults (ported from upstreamBaseFormatModelTests); empty nested struct (every subfield defaulted) still fills.spark-3.1TestSparkOrcReaderDefaults— row reader fills the default when opted in; reads NULL when not opted in (proves the gate).iceberg-orcsuite + Spark 3.1TestSparkOrcReader(complex nested random data) / record round-trip pass.spotlessApplyclean. (CI to run full suites +revapi.)