ORC: bind struct fields by Iceberg field id in generic and Spark 3.5 readers

[cbb330]

Summary

Backports the upstream id-based ORC StructReader binding (apache/iceberg 77e7dbb24, PR #15776) to the openhouse-1.5.2 line. This is the 1.5.x companion to the 1.2.x backport in #256, targeting Spark 3.5.

What changes: the ORC reader used to bind struct fields positionally — consuming ORC column vectors in projection order, assuming projected field order matches the file's physical column order. This PR switches the converted readers to id-based binding: each expected field is matched to its ORC column by its Iceberg field id, robust to field reordering and schema evolution.

Why it's safe: buildOrcProjection already normalizes column order, so id-based binding produces identical results to positional binding on all current inputs. It is behavior-preserving — and the prerequisite for the follow-up work (see Next steps).

Review this after #256 — it is nearly the same change

The ORC core of this PR is byte-identical to #256. Proof (renders "the branches are identical"): cbb330/iceberg-apache@compare/orc-id-binding-shared-core...compare/orc-id-binding-shared-core-1.5.x

So the only thing that genuinely needs fresh review here is the Spark 3.5 wiring. Everything else mirrors #256 (whose inline annotations classify each change).

How id-based binding works (quick primer)

An ORC file stores columns in a physical order. Iceberg has an expected schema, and every field has a permanent numeric field id. Positional binding = "the Nth expected field is the Nth file column" (trusts order); id-based binding = "match each expected field to the file column with the same field id" (order-independent). The real logic lives in the shared OrcValueReaders.StructReader base class; the per-engine reader edits just thread the ORC file schema (TypeDescription) down to it and opt in.

Provenance of each change

Change	Classification
OrcValueReaders, GenericOrcReaders, GenericOrcReader	Backported ~verbatim — byte-identical to #256 (minus the omitted branches below)
SparkOrcValueReaders / SparkOrcReader (v3.5)	Backported with adaptation — same shape; retains the @Deprecated positional overload upstream deleted. Notably SparkOrcReader's one-liner is exactly what upstream main already ships for Spark 3.5 (see divergence comment).
TestGenericOrcReaderIdBinding	Net new, modeled on shipped TestSparkOrcReadMetadataColumns / TestGenericReadProjection

Adaptations vs apache/main

1.5.x lacks the constructs the upstream commit references, so these are intentionally omitted (they would not compile on 1.5.x):

1. Row-lineage — ROW_ID / LAST_UPDATED_SEQUENCE_NUMBER branches, handleRowIdField / handleLastUpdatedSeqField, RowIdReader / LastUpdatedSeqReader.
2. UNKNOWN type clause in the metadata fallback.
3. VARIANT reader/visitor.

Everything else is aligned to upstream verbatim.

Scope

Only GenericOrcReaders and Spark 3.5 SparkOrcValueReaders are converted. The positional constructor/overloads are retained and @Deprecated because the unconverted readers (Spark 3.3/3.4, Flink) still call them.

Reviewing the divergence from upstream

• Upstream source: PR #15776 / commit 77e7dbb24
• Interactive compare vs apache/main: apache/iceberg@main...cbb330:iceberg-apache:compare/orc-id-binding-1.5.x-vs-apache-main (hit Retry if it says "taking too long to generate")
• Embedded, always-renders copy: see the pinned "Divergence vs apache/main" comment on this PR.

Testing Done

• Unit tests added/updated
• Integration tests pass
• Manual testing performed

Validated on JDK 11:

• ./gradlew :iceberg-data:test --tests TestGenericOrcReaderIdBinding — pass (type promotion, reordered projection, _pos / _deleted, idToConstant).
• ./gradlew -DsparkVersions=3.5 -DscalaVersion=2.12 :iceberg-spark:iceberg-spark-3.5_2.12:test (incl. TestSparkOrcReader, TestSparkOrcReadMetadataColumns) — pass.
• ./gradlew :iceberg-orc:revapi — pass; no .palantir/revapi.yml change needed.
• spotlessApply clean; iceberg-orc / iceberg-data / iceberg-spark-3.5_2.12 compile.

Next steps (this is step 1 of 2)

This PR lands only the binding mechanism and does not change read output. The motivating follow-up is column default values: once fields are bound by id, a subsequent change can use those bound ids to resolve and apply an Iceberg field's initial-default value for fields present in the expected schema but absent from the ORC file (which today read as null). Defaults are only correct once binding is keyed on field id rather than position, so this PR is the prerequisite. Defaults will land as a separate PR on top of this one.

[cbb330]

Divergence vs apache/main (embedded)

As of apache/main 470f4642bdda. Always-renders copy of the interactive compare.

ORC core (OrcValueReaders, GenericOrcReaders, GenericOrcReader) is byte-identical to #256 — see the divergence comment there. The deletions are the intentionally-omitted row-lineage / UNKNOWN / VARIANT constructs.

Spark 3.5 (the 1.5.x-specific part)

spark/v3.5/.../SparkOrcValueReaders.java vs apache/main

diff --git a/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/data/SparkOrcValueReaders.java b/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/data/SparkOrcValueReaders.java
index 67664ac6c..47c359732 100644
--- a/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/data/SparkOrcValueReaders.java
+++ b/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/data/SparkOrcValueReaders.java
@@ -70,12 +70,23 @@ public class SparkOrcValueReaders {
     }
   }
 
+  /**
+   * @deprecated Use {@link #struct(TypeDescription, List, Types.StructType, Map)} instead. This
+   *     method uses position-based binding which may cause field misalignment in MOR and lineage
+   *     scenarios.
+   */
+  @Deprecated
   static OrcValueReader<?> struct(
-      TypeDescription record,
+      List<OrcValueReader<?>> readers, Types.StructType struct, Map<Integer, ?> idToConstant) {
+    return new StructReader(readers, struct, idToConstant);
+  }
+
+  static OrcValueReader<?> struct(
+      TypeDescription orcType,
       List<OrcValueReader<?>> readers,
       Types.StructType struct,
       Map<Integer, ?> idToConstant) {
-    return new StructReader(record, readers, struct, idToConstant);
+    return new StructReader(orcType, readers, struct, idToConstant);
   }
 
   static OrcValueReader<?> array(OrcValueReader<?> elementReader) {
@@ -146,12 +157,24 @@ public class SparkOrcValueReaders {
   static class StructReader extends OrcValueReaders.StructReader<InternalRow> {
     private final int numFields;
 
+    /**
+     * @deprecated Use {@link #StructReader(TypeDescription, List, Types.StructType, Map)} instead.
+     *     This constructor uses position-based binding which may cause field misalignment in MOR
+     *     and lineage scenarios.
+     */
+    @Deprecated
+    protected StructReader(
+        List<OrcValueReader<?>> readers, Types.StructType struct, Map<Integer, ?> idToConstant) {
+      super(readers, struct, idToConstant);
+      this.numFields = struct.fields().size();
+    }
+
     protected StructReader(
-        TypeDescription record,
+        TypeDescription orcType,
         List<OrcValueReader<?>> readers,
         Types.StructType struct,
         Map<Integer, ?> idToConstant) {
-      super(record, readers, struct, idToConstant);
+      super(orcType, readers, struct, idToConstant);
       this.numFields = struct.fields().size();
     }
 

spark/v3.5/.../SparkOrcReader.java: zero diff vs apache/main. Upstream's Spark 3.5 reader already threads record into struct(...), so our one-line change is byte-for-byte what upstream already ships.

[cbb330]

Reviewer orientation

Please review #256 (the 1.2.x backport) first. The ORC core of this PR is byte-identical to it, so the per-line backport / adapted / net-new classification lives there as inline review annotations: #256 (review)

For this PR, the only genuinely new surface to review is the Spark 3.5 wiring (SparkOrcReader one-liner + SparkOrcValueReaders overload/ctor). Notably SparkOrcReader's change is a zero-diff vs upstream apache/main (upstream 3.5 already threads record) — see the "Divergence vs apache/main" comment above.

Everything else (the shared ORC core + the equivalence test) is identical to #256: cbb330/iceberg-apache@compare/orc-id-binding-shared-core...compare/orc-id-binding-shared-core-1.5.x

[mkuchenbecker]

Are we not going to achieve parity in parquet? What guards do we have enabling this and are all the tables using orc today?

[mkuchenbecker]

what is the impact on nullable? Bu defautl these mist be nullable, no? Otherwise it will not work with Trino at-all

[mkuchenbecker]

does Trino use the same orc reader logic via import?

[teamurko]

it uses its own readers https://github.com/linkedin-grid/trino/blob/7d4e7662df62e2a3d63d47bea76d012b1d666ac9/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergPageSourceProvider.java#L820, it supports id based mapping

[cbb330]

^ this

I think trino doesn't use any of the code changed in this diff. but I will do some investigation and potentially put up an acc test or do a smoke test in li-oh to confirm that trino doesn't use the apache/iceberg/orc code at runtime.

this goes into the architecture of trino vs other engines. trino manages its own record input/output implementations which must cover all datasources while engines like spark delegate IO to pluggable data sources using DSv2

[mkuchenbecker]

So TLDR this adds the schema to align orc columns to ORC columns

[teamurko]

Confirming that it's the same change as #265
Overall, LGTM, letting @mkuchenbecker to review

[cbb330]

Are we not going to achieve parity in parquet? What guards do we have enabling this and are all the tables using orc today?

good question @mkuchenbecker, I could add the answer on this PR: #263

since there are a lot of open PRs and questions. this PR is only a backport of upstream/main of the underlying properties that default values would need to layer onto.