Repository navigation
fix: match Iceberg column names exactly in the native scan - #7130
dwsmith1983 wants to merge 10 commits into
Conversation
The native Iceberg scan folded column names in two places, though both sides of each match already carry the table schema's names. The schema adapter turned a table with columns `a` and `A` into a duplicate-field error. FileScanTasks were built case-insensitive, and iceberg-rust uses that flag only to bind equality-delete predicates, so `a` and `A` resolved to one field id picked by HashMap order and deletes hit the wrong column. Both now match exactly. iceberg-rust relabels batches and equality-delete columns to the task schema by field id, so an exact match is a field-id match. A top-level column missing from the batch is now an error rather than a column of nulls.
sunchao
left a comment
There was a problem hiding this comment.
Reviewed the entire four-file diff from base 39380cca813bc0b861353de2cb53af960dd2a87f to head 32c8ea856e35ac4c522789c7c976ac7ff83dc54e. The PR is not a draft. No introduced P1/P2 issues found within this review.
Routed skills: review-comet-pr and review-comet-ffi-pr for schema and Arrow boundary checks. Read AGENTS.md, the discussion snapshot, live PR discussion, and linked issues. There were no existing PR reviews, comments, or threads containing unresolved concerns.
Summary
- Prior state and problem: Both the scan adapter and equality-delete binding matched names case-insensitively. Distinct Iceberg columns such as
aandAcould trigger duplicate-column errors or bind deletes to the wrong field. - Design approach: Use exact matching after iceberg-rust has transformed columns to the task schema, and reject a missing projected top-level column instead of silently synthesizing nulls.
- Correctness: Traced data-batch transformation, equality-delete schema evolution, predicate binding, and Comet projection. The pinned iceberg-rust constructs projected names from the task schema and materializes absent optional fields. Its delete loader evolves equality columns by field ID before binding their names. A standalone reader probe passed all four tested projections with exact matching. Restoring insensitive binding reproduced two surviving rows where one was expected.
- Compatibility analysis: Checked Spark’s schema-derived scan attributes across the configured 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0 sources, and field-ID-based equality-delete handling in Iceberg Java 1.5.2, 1.8.1, 1.10.0, and 1.11.0. Exact internal matching preserves Spark’s query-name resolution. Spark 4.2 Iceberg integration tests remain skipped by the existing runtime-compatibility gate. The affected paths on
branch-1.1also use insensitive matching, so this intentionally fixes released behavior. - Key design decisions: Keeping internal matching independent of
spark.sql.caseSensitiveis justified by the canonical task-schema names. The missing-column guard checks the transformed batch, allowing iceberg-rust to continue supplying legitimate schema-evolution nulls. - Implementation sketch:
iceberg_parquet_options()enables exact adapter matching, the planner setsFileScanTask.case_sensitive, and projection construction checks root-column presence. Rust and Scala tests cover case-distinct columns, deletes, query casing, renames, name mapping, and existing nested-rename fallback. Documentation removes the obsolete duplicate-field limitation. - Performance: Exact matching avoids case-folding and schema-remapping work during adapter construction. The added column lookup runs when cached projection expressions are rebuilt, without adding per-row work or buffer copies. No benchmark was run, and no P1/P2 performance regression was identified.
- Design: The change addresses both incorrect matching sites with a consistent invariant. It preserves the existing scan, projection cache, and fallback structure.
- Abstraction & complexity: The small options helper keeps production and unit-test configuration aligned. No additional cross-language configuration, ownership mechanism, or unnecessary abstraction is introduced.
- Behavioral changes worth calling out: Case-distinct columns can be read separately, equality deletes bind to their intended columns, and an unexpectedly missing batch root now raises an error. Existing nested schema-evolution fallback and Arrow ownership rules remain intact.
- Suggested improvements: No additional code change meeting the P1/P2 reporting bar was identified. Comet and Spark/Iceberg integration validation remains outstanding as described below.
Exact-head CI: Label pull requests passed. Comet CI, Check PR Title, and CodeQL report action_required. There is no build or integration-test verdict for this head.
Validation: git diff --check passed. A disposable probe linked against the pinned iceberg-rust libraries verified distinct equality-delete keys, renamed physical columns, an absent optional column, individual-column projections, and zero-column projection. Exact matching returned the expected single surviving row in all four cases. The prior insensitive setting returned two rows in the control case.
Validation limits: The focused Comet Rust test build failed because the installed Java runtime lacks jni.h. A retry without default features timed out after 180 seconds during dependency compilation, before tests ran. The standalone probe validates the upstream reader contract. Comet adapter unit tests and JVM/Spark/Iceberg suites were not executed, and the author’s reported suite passes were not independently confirmed. Project source remains unchanged.
andygrove
left a comment
There was a problem hiding this comment.
Thanks for tracking this down, and for the thorough tests. I traced the exactness claim through the code and it holds at the top level. CometIcebergNativeScan.scala resolves each Spark output attribute to a field id with an exact nameToFieldId.get(attr.name) against the task schema, and iceberg-rust labels the top-level output columns from that same schema. So the names the adapter compares are already exact on both sides, and the new missing-column error should not fire for a legitimate scan.
I compared against branch-1.1, which has the same case-insensitive adapter and with_case_sensitive(false) in the planner. So the a/A read and the equality-delete binding fixes are changes from the released behavior, and I read them as intended.
I have two questions.
I am less sure about nested fields. If I am reading record_batch_transformer.rs in the pinned iceberg-rust correctly, a column whose arrow type equals_datatype the target is passed through unchanged, and that comparison ignores nested field names. So struct children keep the file's physical names, and only top-level names are relabeled. Before this PR, a name-mapped file (add_files or importSparkTable) with a nested child named X read into a table field x matched case-insensitively in match_struct_fields. Now it would find no match. The nested test in this PR covers a case-only rename, which already falls back, but not a name-mapped file with nested physical names that differ in case. Could you add a test for that shape, compared with Spark? If it now reads nulls or fails, should nested matching stay case-insensitive, or should that shape fall back to Spark?
Could the missing-column error also be exercised against the metadata columns (_file, _pos, _partition)? Those resolve through MetadataFieldIds rather than the task schema. A short query selecting _file next to a normal column with spark.sql.caseSensitive=true would show that the new check never fires for them.
CI has not run on this head yet, since the Comet workflow is waiting for approval, so I have not confirmed the suites pass.
…act Iceberg matching A name-mapped file whose nested children differ in case from the table reads the same as before: iceberg-rust hands the adapter a batch with the table's nested names. Metadata columns resolve by reserved field id, so the exact top-level check leaves them alone under either case setting.
Added in 60b4745: a name-mapped file imported with The shape does read wrong values, on main as well, from two problems in iceberg-rust, which maps names for top-level fields only. Any read that also projects a top-level column gets nulls for
Also in 60b4745: |
comphead
left a comment
There was a problem hiding this comment.
Binding the adapter and the task flag exactly matches the exact nameToFieldId lookup on the Scala side, and sunchao's review found no P1 or P2 issues. Three comments on the Scala tests.
| CometConf.COMET_EXEC_ENABLED.key -> "true", | ||
| CometConf.COMET_ICEBERG_NATIVE_ENABLED.key -> "true")(f) | ||
|
|
||
| test("issue 5540: case-sensitive read of columns a and A") { |
There was a problem hiding this comment.
These three tests (case-sensitive read of columns a and A, case-sensitive filters on columns a and A and the case-insensitive control at line 1373) create plain tables and compare queries, with no equality-delete API calls. sql-tests/iceberg already has fixtures that use -- Config: for catalog settings, so a fixture with -- Config: spark.sql.caseSensitive=true and (a INT, A INT) rows could replace about 70 lines of Scala and the failingNativeScanChecks helper. Fixture query statements also report each failure separately. I have not confirmed that the config line is applied before the fixture's CREATE TABLE, though withFixtureConfigs appears to wrap the whole file. The equality-delete tests need commitEqualityDelete and would stay in Scala.
There was a problem hiding this comment.
Moved the read and filter tests to sql-tests/iceberg/case_sensitive_columns.sql in b8518b8. withFixtureConfigs wraps the whole file, so the case setting applies before the CREATE TABLE, and each query checks the answer and that the scan stays native. On main's native code the fixture fails with the duplicate-field error. The case-insensitive control stays in Scala, since it commits an equality delete on Id.
| } | ||
| } | ||
|
|
||
| test("issue 5540: case-insensitive read of columns a and A with equality deletes") { |
There was a problem hiding this comment.
This test and case-sensitive equality deletes keyed on a and on A (line 1349) create the same (a INT, A INT) table, insert the same three rows and commit the same two equality deletes on a and A. Only the read-time setting differs. One test could build the table once, run the named-column queries under true, then run SELECT * and count(*) under false, including the Spark 4.1 COLUMN_ALREADY_EXISTS branch. That drops a table setup and about 25 lines.
There was a problem hiding this comment.
Merged in b8518b8. One test builds the table once, runs the named-column queries with spark.sql.caseSensitive=true, then SELECT * and count(*) with it off, including the 4.1 COLUMN_ALREADY_EXISTS branch.
| } | ||
| } | ||
|
|
||
| test("issue 5540: name mapping onto physical columns that differ in case") { |
There was a problem hiding this comment.
This test and name mapping onto nested children that differ in case (line 1501) repeat the same reflective SparkTableUtil.importSparkTable call of about 15 lines, and the suite already calls importSparkTable through reflection in other places. A small private helper such as importParquetInto(table, sourceTableName, stagingDir) would remove the two new copies.
There was a problem hiding this comment.
Added importParquetInto in b8518b8 for these two. The older call sites look up SparkTableUtil before creating their source table and cancel if it's missing, so I left them as they were.
Read and filter checks for case-variant columns now live in sql-tests/iceberg/case_sensitive_columns.sql. The suite keeps the equality-delete coverage in one test for both key columns and shares a helper for importing Parquet files into a table.
Drop comments that restate the code or a test's name, and shorten the rest to why names match exactly and what the tests guard.
| s"SELECT count(*) FROM $t")) | ||
| } | ||
| // `a` is ambiguous here, so name no column; Spark 4.1+ rejects the duplicates itself. | ||
| withCaseCatalog(warehouseDir, caseSensitive = false) { |
There was a problem hiding this comment.
Now that this read goes native, could CometScanRule (~line 775) use globalFieldIdMapping.get(attr.name)? With the resolver, a and A share one field ID, so a nested rename under A could skip the fallback.
There was a problem hiding this comment.
Yes, and it showed up as wrong rows. On Spark 3.5 and 4.0 with caseSensitive=false, SELECT * over a table with a and A mapped both to a's field id, so the nested-field check never looked at A. A field renamed under A then read back as NULL from files written before the rename.
7497946 switches to globalFieldIdMapping.get(attr.name), the lookup the serde already uses, and adds a test for that table. Spark 4.1 rejects the SELECT * with COLUMN_ALREADY_EXISTS, so on 4.1 the test covers the case-sensitive reads: SELECT A falls back and SELECT a stays native.
| "PARTITIONED BY (Region) TBLPROPERTIES ('format-version' = '2')") | ||
| spark.sql(s"INSERT INTO $t VALUES (1, 'a', 10), (2, 'a', 20), (3, 'b', 30)") | ||
| } | ||
| // Metadata columns resolve by reserved field id, so the exact name check must skip them. |
There was a problem hiding this comment.
nit: does the native check actually skip metadata columns, or do they just pass because the names match? Maybe:
| // Metadata columns resolve by reserved field id, so the exact name check must skip them. | |
| // Metadata columns resolve by reserved field ID and arrive with Spark's reserved names, | |
| // so they also pass the exact-name check. |
There was a problem hiding this comment.
It doesn't skip them. build_projection_expressions requires every projected name in the batch, and iceberg-rust emits _file, _pos, _spec_id and _partition under those names, so they pass the same way data columns do. Reworded in 7497946.
With spark.sql.caseSensitive=false the rule matched projected columns to field ids through the session resolver, so in a table with columns a and A both resolved to a's id. The nested-field check then skipped A, and on Spark 3.5 and 4.0 a field renamed under A read back as NULL from files written before the rename. Look ids up by exact name, as the serde does.
Which issue does this PR close?
Closes #5540.
Closes #7129.
Rationale for this change
The native Iceberg scan matched column names case-insensitively, though iceberg-rust batches and Spark's scan attributes already carry the table's exact names. A table with columns
aandAfailed with a duplicate-field error where Spark reads it. Equality deletes could also bind to the wrong one of the two columns, bringing deleted rows back.What changes are included in this PR?
spark.sql.caseSensitivesays.CometScanRulelooks up projected field ids by exact name, as the serde does, so the nested-field fallback checksAapart froma.How are these changes tested?
A new SQL file test and new
CometIcebergNativeSuitetests compare against Spark for tables with columns that differ only in case, covering reads, filters, equality deletes, case-only renames, name-mapped files and metadata columns. Rust unit tests cover the adapter and the missing-column error. One test renames a nested field underAbesidea: on Spark 3.5 and 4.0 a case-insensitiveSELECT *must fall back (it returned NULL for the renamed field before), and withcaseSensitive=trueevery version falls back onSELECT Aand keepsSELECT anative.#7028 replaces the nested-rename fallback that test expects, so whichever of the two lands second adjusts it.