[claudevdm]

With schema_evolution_options and location_prefix set, AddFiles can register Hive-style layouts where a partition value exists only in the folder name:

gs://lake/orders/dt=2024-01-15/part-0.parquet   # the file has no dt column

If the table declares dt required and is partitioned by dt as is (identity), schema evolution treated dt as a required column missing from the file, so every row would read null. That meant:

• with ALLOW_FIELD_ADDITION only, the pipeline failed (FAIL_PIPELINE) or every file went to the error output (ROUTE_TO_ERRORS);
• with ALLOW_FIELD_RELAXATION, dt was made optional;
• with dt in required_columns, the pipeline failed or every file was refused, even with ALLOW_FIELD_RELAXATION.

These files don't read null for dt. Per the Iceberg spec (column projection), a column missing from a data file is read from the file's partition value when an identity partition exists for it. AddFiles sets that value from the folder name.

Change

When location_prefix is set and the table has an identity partition on a column, a file whose path gives that column a value (a dt=... folder other than dt=__HIVE_DEFAULT_PARTITION__) no longer counts as missing it:

• Pre-pass: ReadFooterSchema records the partition keys in each file's path, CollectDistinctSchemas keeps the keys every file of a schema has, and SchemaPlan passes the matching column ids to SchemaDelta.
• Registration: the per-file coverage check and the required_columns check do the same from the file's own path. The exemption only applies when the file lacks the column; a file that has dt with nulls in it is still refused.
• SchemaDelta.classify takes a set of columns filled from outside the file and doesn't report them as needing to become optional.
• New PartitionPaths helper; the existing prefix cut moved into it unchanged.

Required dt, files under dt=.../ without a dt column	Before	After
ALLOW_FIELD_ADDITION only	Pipeline fails / every file refused	Registers; dt stays required
With ALLOW_FIELD_RELAXATION	Registers; dt made optional	Registers; dt stays required
dt in required_columns	Pipeline fails / every file refused	Registers; dt stays required
Plus one file directly under the prefix (no dt value)	That file: pipeline fails, file refused, or dt made optional with relaxation	Same for that file; under ROUTE_TO_ERRORS the files in dt= folders now register

Unchanged:

• Schema evolution off. Only the existing prefix cut runs, moved unchanged. Running the same eight scenarios on master and on this branch gave identical pipeline graphs, registered files, partitions and error rows.
• No location_prefix, or an unpartitioned table.
• Creating a missing table.

[claudevdm]

/Gemini review

[claudevdm]

/gemini review

[github-actions]

📋 Review Summary

This PR adds support for reading partition column values from Hive-style folder paths (key=value/) during AddFiles, allowing data files to omit identity-partitioned columns even when those columns are required or pinned in the destination Iceberg table. However, the PR currently contains a critical merge/rebase regression: the first commit (a3785bdc) introduced the feature, while the subsequent commits (de7f78a5 and f50f71d6) were based on master rather than the feature commit, leaving only a 1-line change in .github/trigger_files/IO_Iceberg_Integration_Tests.json in the PR diff.

🔍 General Feedback

• Git History / Missing Changes (🔴 Critical): Commit de7f78a5 (revert-doc) appears to have been created on master (or reverted the entire feature commit instead of just the doc change in AddFilesSchemaTransformProvider.java), so the net diff of this PR against apache:master is currently just .github/trigger_files/IO_Iceberg_Integration_Tests.json. Please cherry-pick or rebase a3785bdc79b5e914a1a28a5a78d677ccb6dc5436 back onto the branch.
• Implementation Quality (on a3785bdc): The feature implementation across PartitionPaths, CollectDistinctSchemas, ReadFooterSchema, SchemaDelta, SchemaPlan, and AddFiles is well-structured, cleanly handles __HIVE_DEFAULT_PARTITION__ and non-identity partition transforms, and has thorough unit and integration test coverage.
• Edge Case in SchemaDelta.classify (🟡 Medium): In SchemaDelta.classify(Table, Schema, Schema, Set<Integer>) (from a3785bdc), the early return if (before.sameSchema(fileSchema)) does not check ColumnNameChecks.problem(fileSchema), whereas a schema differing only by a path-supplied partition column falls through to ColumnNameChecks.problem(fileSchema). Even More importantly, if before has a required identity-partitioned column dt and fileSchema omits dt while filledOutsideFile contains dt's field ID, findAbsentRequired skips dt (absent is empty), but classify still runs unionByNameWith(fileSchema) and diffStruct(before.asStruct(), merged.asStruct(), ...)—which works because unionByNameWith does not relax missing columns unless makeOptional is called, though it still opens a scratch transaction and runs unionByNameWith unnecessarily when the only difference is path-supplied columns.

---

💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.

[claudevdm]

/gemini review

[github-actions]

📋 Review Summary

This PR introduces partition-path awareness for schema evolution, a dry-run reporting mode (DryRunReport), and Parquet field-ID conflict validation (ParquetFieldIds) for AddFiles. The overall design is well-structured and cleanly separates concerns across the pre-pass, commit plan, and per-file registration stages.

🔍 General Feedback

• There are a couple of edge cases in ParquetFieldIds and AddFiles where Parquet schemas with duplicate field IDs or pinned identity-partition columns provided via path can cause unexpected failures or false rejections.
• URL-encoded partition key names in Hive-style folder paths are not decoded before matching against Iceberg PartitionSpec field names.

---

💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.

💬 Inline Findings

• sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/AddFiles.java (line 717): 🟠 High: checkPinnedColumns does not account for filledByPartitionPath (unlike SchemaDelta.classify right above it). If a required/pinned identity-partition column is provided by the file's directory path (e.g., dt=2024-01-15/f.parquet) and omitted from the Parquet file itself, SchemaPlan and SchemaDelta accept the file, but checkPinnedColumns will reject it here with missing required column because fileSchema.findField(fileColumn) returns null. Columns whose table field ID is in filledByPartitionPath should be skipped in checkPinnedColumns.

• sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/ParquetFieldIds.java (line 122): 🟠 High: TypeUtil.indexById and fileSchema.findColumnName(id) can throw IllegalArgumentException (from Guava ImmutableMap.Builder duplicate key conflict when two columns in the file share the same Parquet field ID) or fail if a nested struct/list/map shares an ID with another field. Because this call is outside the try / catch (RuntimeException e) block on lines 116-120, a file with duplicate field IDs will throw an unhandled IllegalArgumentException instead of a ConflictException, crashing the bundle rather than routing the file to the error output.

• sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/PartitionPaths.java (line 62): 🟡 Medium: Hive-style partition paths URL-encode special characters in both keys and values (and TableUtils.parsePartitionPath uses URLDecoder.decode(part, "UTF-8")). Extracting parts[0] and comparing HIVE_NULL.equals(parts[1]) without URL-decoding means partition field names containing special characters (like spaces or colons) won't match PartitionField.name() in columnsFilledByKeys.

[claudevdm]

R: @ahmedabu98

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (76f6e6d) to head (f50f71d).
⚠️ Report is 256 commits behind head on master.

Additional details and impacted files

@@              Coverage Diff              @@
##             master   #40691       +/-   ##
=============================================
- Coverage     56.16%        0   -56.17%     
=============================================
  Files          1124        0     -1124     
  Lines        178336        0   -178336     
  Branches       1489        0     -1489     
=============================================
- Hits         100170        0   -100170     
+ Misses        75645        0    -75645     
+ Partials       2521        0     -2521     

Flag	Coverage Δ
go	?
java	?
python	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment assign set of reviewers

[ahmedabu98]

thanks, this LGTM!
just a nit regarding naming to avoid confusion for future readers/agents

also maybe should have an end-to-end test with /dt=__HIVE_DEFAULT_PARTITION__/

[ahmedabu98]

Ahh sorry looks like my comments didnt get thru last time