ORC: bind struct fields by Iceberg field id in generic and Spark 3.1 readers - #265
ORC: bind struct fields by Iceberg field id in generic and Spark 3.1 readers#265cbb330 wants to merge 1 commit into
Conversation
bd16f75 to
d39cbfd
Compare
cbb330
left a comment
cbb330
left a comment
There was a problem hiding this comment.
Provenance annotations for each code block, categorized as: backported exactly / backported with adaptations / net new / inspiration from shipped code. Reference: apache/iceberg 77e7dbb24 (PR #15776).
These six files are byte-identical to the earlier 1.2.x-raised backport in #256 (same patch ID 74c67920ff84); this PR re-bases that work onto openhouse-1.2.0, which never had ID-bound ORC readers.
| // Maps each projected struct field position to the matching child index in the ORC schema. | ||
| // This allows fields to be read by Iceberg field ID when the projected struct order differs | ||
| // from the file schema. | ||
| private final int[] orcFieldIndex; |
There was a problem hiding this comment.
Backported with adaptations. The orcFieldIndex field is from upstream's StructReader. The doc-comment explaining the null sentinel is reworded for this fork to make explicit that positional binding is preserved for unconverted engine readers.
| * This constructor uses position-based binding which may cause field misalignment in MOR | ||
| * scenarios. | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
Backported with adaptations. Upstream keeps the positional constructor @Deprecated too; we mirror that (Spark 2.4/3.2/3.3 and Flink still call it on this line). Adaptation: this.orcFieldIndex = null; just below is the sentinel that makes readInternal fall back to exact positional behavior.
| } | ||
| } | ||
|
|
||
| protected StructReader( |
There was a problem hiding this comment.
Backported exactly (minus deliberately omitted branches). This ID-binding constructor matches the upstream StructReader line-for-line, except the row-lineage branches (ROW_ID / LAST_UPDATED_SEQUENCE_NUMBER plus their handle* helpers) and the || field.type().typeId() == Type.TypeID.UNKNOWN clause are dropped — neither construct exists on openhouse-1.2.0.
| this.readers[pos] = constants(false); | ||
| } else if (fileReader != null) { | ||
| this.isConstantOrMetadataField[pos] = false; | ||
| this.orcFieldIndex[pos] = fieldIdToOrcIndex.getOrDefault(field.fieldId(), -1); |
There was a problem hiding this comment.
Backported exactly. The orcFieldIndex[pos] assignment is the core of ID binding: the expected field at pos records which ORC child index carries its field ID, so readInternal can select the right vector regardless of physical order.
| } | ||
| } | ||
|
|
||
| private Map<Integer, Integer> buildFieldIdToOrcIndex(TypeDescription orcType) { |
There was a problem hiding this comment.
Backported exactly. buildFieldIdToOrcIndex is verbatim from upstream — maps each ORC child's embedded Iceberg field ID to its physical index.
| List<String> names, | ||
| List<OrcValueReader<?>> fields) { | ||
| return GenericOrcReaders.struct(fields, expected, idToConstant); | ||
| return GenericOrcReaders.struct(record, fields, expected, idToConstant); |
There was a problem hiding this comment.
Backported exactly. Visitor call site now passes the record TypeDescription — identical one-line change to upstream (record was already provided by the visitor).
| return new StructReader(readers, struct, idToConstant); | ||
| } | ||
|
|
||
| static OrcValueReader<?> struct( |
There was a problem hiding this comment.
Backported with adaptations. Mirrors upstream's 4-arg struct. Adaptation: upstream (Spark v3.5+) deletes the positional overload; here it is retained just above as @Deprecated because Spark 2.4/3.2/3.3 and Flink still use it.
| this.numFields = struct.fields().size(); | ||
| } | ||
|
|
||
| protected StructReader( |
There was a problem hiding this comment.
Backported with adaptations. 4-arg subclass constructor mirroring upstream; the positional subclass constructor is retained @Deprecated just above.
| List<String> names, | ||
| List<OrcValueReader<?>> fields) { | ||
| return SparkOrcValueReaders.struct(fields, expected, idToConstant); | ||
| return SparkOrcValueReaders.struct(record, fields, expected, idToConstant); |
There was a problem hiding this comment.
Backported exactly. Visitor call site passes record — identical to upstream, applied to the 3.1 file. Note upstream's Spark 3.5 reader already ships exactly this line.
| * exercise branches of the id-binding {@code StructReader} constructor that the shared projection | ||
| * tests do not, and assert that id-binding produces the same results positional binding would. | ||
| */ | ||
| public class TestGenericOrcReaderIdBinding { |
There was a problem hiding this comment.
Net new, using inspiration from shipped tests. Not part of the upstream commit. The metadata-column approach (project without metadata fields, hand the reader the full schema including _pos / _deleted) is modeled on the shipped spark/v3.1/.../TestSparkOrcReadMetadataColumns.java; the write/project/read harness follows TestGenericReadProjection / TestGenericData. Covers type promotion, reordered projection, metadata columns, and idToConstant.
Divergence vs
|
Noise-free compares vs
|
…readers Backport the upstream id-based StructReader binding (apache/iceberg 77e7dbb, PR #15776) to the openhouse-1.2.0 ORC reader, adapted for the 1.2.x type system. The ORC StructReader previously bound struct fields positionally, consuming column vectors in projection order. This replaces that with id-based binding: each expected field is matched to its ORC column by Iceberg field id, which is resilient to field reordering and schema evolution. Scope: only GenericOrcReaders and Spark 3.1 SparkOrcValueReaders are converted. The positional constructor/overloads are retained and marked @deprecated because the other engine readers (Spark 2.4/3.2/3.3, Flink) still use them. Row-lineage, UNKNOWN, and VARIANT constructs from the upstream commit are omitted as they do not exist on openhouse-1.2.0. Behavior-preserving for well-formed reads: id-based binding produces identical results to positional binding on all current inputs. Co-authored-by: Cursor <cursoragent@cursor.com>
d39cbfd to
4ca1b1e
Compare
1 participant
Summary
Backports the upstream ID-based ORC
StructReaderbinding (apache/iceberg77e7dbb24, PR #15776) to theopenhouse-1.2.0line, adapted for the 1.2.x type system.What changes: the ORC reader bound struct fields positionally — consuming ORC column vectors in projection order, assuming projected field order matches the file's physical column order. This switches the converted readers to ID-based binding: each expected field is matched to its ORC column by its Iceberg field ID, which is robust to field reordering and schema evolution.
Why it's safe:
buildOrcProjectionalready normalizes column order, so ID-based binding produces identical results to positional binding on all current inputs. It is a behavior-preserving robustness change — and the prerequisite for the default-fill work stacked on top.How ID-based binding works (quick primer)
An ORC file stores columns in a physical order. Iceberg has an expected schema, and every field carries a permanent numeric field ID. Two ways to match expected fields to file columns:
The logic lives in the shared
OrcValueReaders.StructReaderbase class iniceberg-orc; the per-engine reader edits are thin plumbing that thread the ORC file schema (TypeDescription) down to it and opt in.Provenance of each change
OrcValueReaders: ID-binding constructor +readersByFieldId/buildFieldIdToOrcIndex+readInternalselectionGenericOrcReaders4-argstruct+ subclass ctor;GenericOrcReaderrecordthreadingSparkOrcValueReaders/SparkOrcReader(v3.1)@Deprecatedpositional overload that upstream deletedTestGenericOrcReaderIdBindingTestSparkOrcReadMetadataColumns/TestGenericReadProjectionPer-line classification is in the inline review annotations on this PR.
Adaptations vs
apache/main(why it diverges at all)openhouse-1.2.0lacks constructs the upstream commit references, so these are intentionally omitted (porting them verbatim would not compile here):ROW_ID/LAST_UPDATED_SEQUENCE_NUMBERbranches, thehandleRowIdField/handleLastUpdatedSeqFieldhelpers, and theRowIdReader/LastUpdatedSeqReaderclasses.UNKNOWNtype — the|| field.type().typeId() == Type.TypeID.UNKNOWNclause in the metadata fallback.VARIANT— the variant reader/visitor.Everything else is aligned to upstream verbatim (method ordering, Javadoc, whitespace, no defensive null-guards).
Scope
Only
GenericOrcReadersand Spark 3.1SparkOrcValueReadersare converted. The positional constructor/overloads are retained and@Deprecatedbecause the unconverted readers (Spark 2.4/3.2/3.3, Flink 1.14/1.15/1.16) still call them — those readers keep identical positional behavior.Reviewing the divergence from upstream
77e7dbb24apache/main: apache/iceberg@main...cbb330:iceberg-apache:compare/orc-id-binding-vs-apache-main (if GitHub says "taking too long to generate," hit Retry — a large-repo cross-fork rendering quirk)apache/main" comment below.git fetch apache main && git diff apache/main -- orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.javaThese
compare/orc-id-binding-*branches are disposable comparison artifacts, not intended for merge. They remain accurate for this PR: the six files here are byte-identical to the earlier 1.2.x-based backport in #256 (same patch ID74c67920ff84).Companion PR
The identical change is backported to the 1.5.x line (targeting Spark 3.5) in #257. The ORC core is byte-identical between the two backports — proof (renders "the branches are identical"): cbb330/iceberg-apache@compare/orc-id-binding-shared-core...compare/orc-id-binding-shared-core-1.5.x
History
This supersedes #256, which raised the same change against
1.2.x— a near-vanilla apache branch that is not the OpenHouse dev line. #256's closing note incorrectly stated thatopenhouse-1.2.0already had ID-bound ORC readers; it did not (OrcValueReaders.javathere was byte-identical to vanilla 1.2.x). This PR restores that work on the correct base. Note that shanthoosh's #250 (theNestedFieldcolumn-default API, already merged here) has zero file overlap with this change.Stack
StructReaderbackport (base:openhouse-1.2.0)The default-fill PRs build on this binding so physical columns and absent-field default constants are both associated by Iceberg field ID.
Testing Done
TestGenericOrcReaderIdBindingcovers type promotion (int→long, float→double, decimal widening), reordered projection,_pos/_deletedmetadata columns, andidToConstantNext steps
This PR lands only the binding mechanism and deliberately does not change read output. The motivating follow-up is column default values: once fields are bound by ID, the stacked PRs use those bound IDs to resolve an Iceberg field's initial-default for fields present in the expected schema but absent from the ORC file (which today read as
null).Made with Cursor