Addfiles partition bounds fixes by claudevdm · Pull Request #40252 · apache/beam

claudevdm · 2026-09-23T20:36:13Z

When a file is registered without location_prefix, AddFiles infers its partition from the column bounds (min/max) in the Parquet footer. Today that inference, and the bounds themselves, go wrong in several cases: some files are registered under the wrong partition with no error, some fail the whole pipeline, and some are refused for no good reason. The bounds bug also affects unpartitioned tables, because query planners use the stored bounds to skip files.

This PR:

stores column bounds in the unit of the table column's type (millis and nanos timestamps and times, unsigned 32-bit ints, v3 timestamp_ns columns);
puts a file in the null partition only when its partition column is proven null, and refuses (error row) files whose partition can't be determined;
sets the inferred partition as values instead of round-tripping it through a path string.

Wrong identity partitions are worse than they look: Iceberg's readers take an identity-partitioned column's value from the partition tuple, not from the file. On master, a file written without statistics with flag = [true, true] is registered under flag=false, and every reader returns [false, false].

What changes for users

Partition inference (partitioned tables, no location_prefix):

Case	master	this PR
Partition column all null	boolean -> `false`, string -> `"null"`, date/int -> refused (`Text 'null' could not be parsed`)	null partition
File lacks the partition column (written before it existed)	boolean -> `false`	null partition
Partition column without statistics (writer stats off, INT96)	treated as all null, as above	error row: no column bounds, use a location prefix
Partition column with both nulls and values	registered under the value's partition: `IS NULL` misses the null rows, `= value` returns them	error row
Avro file	`NullPointerException` fails the pipeline	error row
`year(ts)`	stored as ordinal 2024 (year 3994)	correct
`month`, `hour` or `identity` on a timestamp	refused (`For input string: "2024-01"`)	registered
Table metrics mode `none`, file has a uint32 or millis `time` column	`ClassCastException` fails the pipeline	works

Column bounds (all tables):

File column	master	this PR
Timestamp in millis or nanos, time in nanos	raw file unit, off by 1000x: range filters can skip the file, `day(ts)` is wrong	table column's unit
Time in millis (INT32), unsigned INT32	`ClassCastException`, file refused	bounds computed (under a `time` / `long` column)
Millis or micros timestamp under a v3 `timestamp_ns` column	10^6x / 1000x too small	nanos

The first commit only adds tests (all but one control fail on master); each later commit makes a group of them pass and names them in its message.

Regression tests (new AddFilesMetricsTest). Bounds tests call getFileMetrics directly. Partition tests run AddFiles in a TestPipeline and check the error output and the committed DataFile's partition. Test files have no field IDs, like Spark or pyarrow output, so they resolve through the name mapping.
Never guess a partition for a column without bounds. Missing bounds used to mean "all null". Now the null partition needs proof: an empty file, a null count equal to the value count (allValuesNull), or a Parquet file without the column (lacksColumn; such a column reads as null). Anything else is an unknown partition. Bound maps are read null-safely for Avro (orEmpty). Worth checking: the null partition is chosen only when every row is null.
Set the partition as values. getPartitionFromMetrics returns the PartitionKey, and the DataFile is built with withPartition instead of withPartitionPath(pk.toPath()). The text round-trip wrote null as null (parsed back as false or the string "null") and couldn't parse month, hour or timestamp values. The location_prefix path is unchanged. SerializableDataFile already carries the partition as JSON, so the null reaches the commit.
Second metrics pass for partition columns only. When the table's metrics mode drops a partition column's bounds, AddFiles computes them from the same footer for inference only (they aren't stored). That pass now defaults every other column to none, so a column Iceberg can't handle no longer fails the bundle.
Bounds in the table column's unit (BoundAdjustment). Iceberg's converter types every Parquet timestamp and time as micros and uint32 as long, but copies the raw statistic. The conversion is chosen from the file's annotation and the table column's type, which getFileMetrics now receives: x1000, /1000 (lower rounded down, upper up), x10^6, or unsigned. INT32 columns that would throw (millis time, uint32) are shown to Iceberg without their annotation so it computes int bounds, then converted. A bound that overflows the table's unit (e.g. year 9999 in nanos) is dropped. If the table column isn't the matching type (e.g. a timestamp file column under a long), the bounds stay as Iceberg computes them, as on master. Worth checking: the annotation stripping, the rounding direction and the overflow handling.
Nulls and values. Bounds ignore nulls, so a partition column holding both is now an unknown partition. The void transform is exempt: it maps everything to null.

Part of #40144

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

…ion inference These tests describe the correct behavior and fail on master; the commits that follow fix them one group at a time. Failures at this commit: Bounds (getFileMetrics): - millis and nanos timestamps, nanos time: bounds stored in the file's unit under a micros type, off by 1000x. - millis time and uint32: ClassCastException (Integer to Long) while collecting bounds. Partition inference without a location prefix: - all-null boolean partition column is registered under flag=false, an all-null string column under the string "null", and a file lacking the column under flag=false: the null partition goes through a path string and is parsed back. - all-null timestamp under day(ts) is refused: "Text 'null' could not be parsed". - a column with statistics switched off is registered under flag=false although every value is true; an INT96 column is refused with the same parse error instead of an unknown partition. - an Avro file on a partitioned table fails the pipeline with a NullPointerException (null bound maps). - a millis timestamp file lands in day 19 (1970) instead of 2024-01-01. - with write.metadata.metrics.default=none, an unrelated uint32 column fails the pipeline with a ClassCastException during the partition recollection. testMicrosTimestampBoundsAreUnchanged passes and stays as the control.

Metrics-based partition inference treated a partition source column with no bounds as all null. That holds for a column whose values are all null, but bounds are also missing when statistics are switched off, for INT96 timestamps (Iceberg collects nothing for them) and for Avro files, whose Metrics carry null bound maps. - An Avro file on a partitioned table failed the bundle with a NullPointerException on metrics.lowerBounds().keySet(), so one Avro file failed the job. Null bound maps are now read as empty. - A column without bounds is an unknown partition (error row naming the column and pointing at the location prefix) unless the null partition is proven: the file is empty, the column's null count equals its value count, or the Parquet file does not contain the column at all (a file written before the column existed reads as null everywhere). Turns green: testAvroFileOnAPartitionedTableIsAnUnknownPartition, testPartitionColumnWithoutStatisticsIsAnUnknownPartition, testInt96PartitionColumnIsAnUnknownPartition.

… string getPartitionFromMetrics returned PartitionKey.toPath() and the DataFile was built with withPartitionPath, which parses each value back from text. A null partition value is written as the text "null", so: - boolean identity partition: Boolean.valueOf("null") files the data under flag=false, and queries pruning on the partition return wrong rows; - string identity partition: the file lands under the string "null"; - int, date and timestamp partitions: the parse fails and the file is refused ("Text 'null' could not be parsed at index 0"). getPartitionFromMetrics now returns the PartitionKey and the DataFile is built with withPartition. The location-prefix option still goes through the path, where Iceberg maps __HIVE_DEFAULT_PARTITION__ to null. Turns green: the all-null boolean, string and day(timestamp) tests and testFileLackingThePartitionColumnRegistersUnderTheNullPartition.

When the file's own metrics lack bounds for a partition source column (e.g. write.metadata.metrics.default=none), the partition is inferred from metrics recollected with "full" for the partition columns. Every other column fell back to Iceberg's default mode, so bounds were computed for the whole file, and a column Iceberg cannot collect bounds for (uint32, millis time: ClassCastException) failed the bundle from a code path that only catches UnknownPartitionException. The recollection now sets the default mode to none, so only the partition columns are read. It is also less work on wide files. Turns green: testUnrelatedColumnDoesNotBreakInferenceUnderMetricsModeNone.

Files without field ids resolve through the name mapping, and their column bounds are what partition inference and query pruning read, so they must be encoded in the Iceberg type's unit. Iceberg's converter maps TIMESTAMP/TIME in MILLIS or NANOS to micros types and uint32 to long, but ParquetUtil.footerMetrics stores the file's raw statistic: - TIMESTAMP(MILLIS|NANOS), TIME(NANOS): bounds off by 1000x, silently. A 2024 millis timestamp gets a bound in 1970; day(ts) puts the file in the wrong partition and range predicates prune it away, while the data reads correctly. - TIME(MILLIS) int32 and UINT32: ClassCastException (Integer to Long) while collecting bounds, so the file is routed to errors. Fix: BoundAdjustment, applied in getFileMetrics after the id mapping. Affected int32 columns are presented to Iceberg without their annotation so it computes plain int bounds instead of throwing; every affected lower/upper bound is then rewritten as an 8-byte long in the Iceberg unit (millis x1000; nanos /1000 with lower rounded down and upper up so bounds stay conservative; uint32 via toUnsignedLong). Counts are untouched. Files whose columns need no adjustment take the old path unchanged. Turns green: the six bounds tests' remaining failures and testMillisTimestampFileLandsInItsDayPartition.

…alues Found while fixing the partition inference; not in the fuzz findings. Column bounds ignore nulls, so a partition source column holding true and null has bounds true..true and the file was registered under flag=true. Its null rows belong to the null partition, so a query pruning on the partition silently loses them. Such a file spans two partitions and is now an unknown partition (error row naming the column). A transform that maps every value to null (void) is unaffected: nulls and values land in the same partition. Test and fix together: on the previous commit testPartitionColumnWithNullsAndValuesIsAnUnknownPartition fails with the file registered and no error row.

claudevdm · 2026-09-23T20:38:29Z

R: @ahmedabu98

github-actions · 2026-09-23T20:39:03Z

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 · 2026-09-25T22:57:01Z

+      return org.apache.parquet.schema.Types.primitive(
+              primitive.getPrimitiveTypeName(), primitive.getRepetition())
+          .id(primitive.getId().intValue())
+          .named(primitive.getName());


AI review complained that this creates an issue when (1) the file has multiple row groups, and (2) one row group's bound is a small unsigned int and the other row group's bound is a very large unsigned int (2^31 and above).

This block recreates the type without preserving the unsigned info, so Iceberg treats it as a signed int. When Iceberg tries merging row groups, it compares their bounds to create one pair of bounds for the file. In this case, the comparison can interpret large unsigned ints (e.g. 2147483649) as negative signed ints (-2147483647) and output incorrect bounds.

We can trust the min/max bounds that Parquet shows for each row group, but we can't trust Iceberg's min/max merge output across row groups in this case. We should still strip the annotation but we'd need to compute the bounds of unsigned int columns ourselves (using Integer.compareUnsigned).

Maybe also include a test with two row groups to confirm the failure and also make sure the fix works as intended

We now reject a file whose uint32 column holds a value of 2^31 or more. Iceberg's readers read uint32 values as signed, so 3000000000 comes back as -1294967296. Comparing the row-group min/max values as unsigned would give bounds that don't match what readers return.

Below 2^31 signed and unsigned order are the same, so the bounds Iceberg computes are correct and we keep them.

codecov · 2026-09-28T22:27:02Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.61%. Comparing base (d1ebefb) to head (d876b7f).
⚠️ Report is 45 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #40252      +/-   ##
============================================
+ Coverage     57.29%   58.61%   +1.32%     
- Complexity     3696    13791   +10095     
============================================
  Files          1201     2579    +1378     
  Lines        194397   270227   +75830     
  Branches       3889    11192    +7303     
============================================
+ Hits         111373   158391   +47018     
- Misses        79416   105802   +26386     
- Partials       3608     6034    +2426

Flag	Coverage Δ
java	`64.97% <ø> (-7.11%)`	⬇️

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.

ahmedabu98

LGTM, thanks!

* AddFiles: regression tests for column bounds and metrics-based partition inference These tests describe the correct behavior and fail on master; the commits that follow fix them one group at a time. Failures at this commit: Bounds (getFileMetrics): - millis and nanos timestamps, nanos time: bounds stored in the file's unit under a micros type, off by 1000x. - millis time and uint32: ClassCastException (Integer to Long) while collecting bounds. Partition inference without a location prefix: - all-null boolean partition column is registered under flag=false, an all-null string column under the string "null", and a file lacking the column under flag=false: the null partition goes through a path string and is parsed back. - all-null timestamp under day(ts) is refused: "Text 'null' could not be parsed". - a column with statistics switched off is registered under flag=false although every value is true; an INT96 column is refused with the same parse error instead of an unknown partition. - an Avro file on a partitioned table fails the pipeline with a NullPointerException (null bound maps). - a millis timestamp file lands in day 19 (1970) instead of 2024-01-01. - with write.metadata.metrics.default=none, an unrelated uint32 column fails the pipeline with a ClassCastException during the partition recollection. testMicrosTimestampBoundsAreUnchanged passes and stays as the control. * AddFiles: never guess a partition for a column without bounds Metrics-based partition inference treated a partition source column with no bounds as all null. That holds for a column whose values are all null, but bounds are also missing when statistics are switched off, for INT96 timestamps (Iceberg collects nothing for them) and for Avro files, whose Metrics carry null bound maps. - An Avro file on a partitioned table failed the bundle with a NullPointerException on metrics.lowerBounds().keySet(), so one Avro file failed the job. Null bound maps are now read as empty. - A column without bounds is an unknown partition (error row naming the column and pointing at the location prefix) unless the null partition is proven: the file is empty, the column's null count equals its value count, or the Parquet file does not contain the column at all (a file written before the column existed reads as null everywhere). Turns green: testAvroFileOnAPartitionedTableIsAnUnknownPartition, testPartitionColumnWithoutStatisticsIsAnUnknownPartition, testInt96PartitionColumnIsAnUnknownPartition. * AddFiles: set the metrics-inferred partition as values, not as a path string getPartitionFromMetrics returned PartitionKey.toPath() and the DataFile was built with withPartitionPath, which parses each value back from text. A null partition value is written as the text "null", so: - boolean identity partition: Boolean.valueOf("null") files the data under flag=false, and queries pruning on the partition return wrong rows; - string identity partition: the file lands under the string "null"; - int, date and timestamp partitions: the parse fails and the file is refused ("Text 'null' could not be parsed at index 0"). getPartitionFromMetrics now returns the PartitionKey and the DataFile is built with withPartition. The location-prefix option still goes through the path, where Iceberg maps __HIVE_DEFAULT_PARTITION__ to null. Turns green: the all-null boolean, string and day(timestamp) tests and testFileLackingThePartitionColumnRegistersUnderTheNullPartition. * AddFiles: recollect partition metrics for the partition columns only When the file's own metrics lack bounds for a partition source column (e.g. write.metadata.metrics.default=none), the partition is inferred from metrics recollected with "full" for the partition columns. Every other column fell back to Iceberg's default mode, so bounds were computed for the whole file, and a column Iceberg cannot collect bounds for (uint32, millis time: ClassCastException) failed the bundle from a code path that only catches UnknownPartitionException. The recollection now sets the default mode to none, so only the partition columns are read. It is also less work on wide files. Turns green: testUnrelatedColumnDoesNotBreakInferenceUnderMetricsModeNone. * AddFiles: store column bounds in the unit of the Iceberg type Files without field ids resolve through the name mapping, and their column bounds are what partition inference and query pruning read, so they must be encoded in the Iceberg type's unit. Iceberg's converter maps TIMESTAMP/TIME in MILLIS or NANOS to micros types and uint32 to long, but ParquetUtil.footerMetrics stores the file's raw statistic: - TIMESTAMP(MILLIS|NANOS), TIME(NANOS): bounds off by 1000x, silently. A 2024 millis timestamp gets a bound in 1970; day(ts) puts the file in the wrong partition and range predicates prune it away, while the data reads correctly. - TIME(MILLIS) int32 and UINT32: ClassCastException (Integer to Long) while collecting bounds, so the file is routed to errors. Fix: BoundAdjustment, applied in getFileMetrics after the id mapping. Affected int32 columns are presented to Iceberg without their annotation so it computes plain int bounds instead of throwing; every affected lower/upper bound is then rewritten as an 8-byte long in the Iceberg unit (millis x1000; nanos /1000 with lower rounded down and upper up so bounds stay conservative; uint32 via toUnsignedLong). Counts are untouched. Files whose columns need no adjustment take the old path unchanged. Turns green: the six bounds tests' remaining failures and testMillisTimestampFileLandsInItsDayPartition. * AddFiles: refuse a file whose partition column holds both nulls and values Found while fixing the partition inference; not in the fuzz findings. Column bounds ignore nulls, so a partition source column holding true and null has bounds true..true and the file was registered under flag=true. Its null rows belong to the null partition, so a query pruning on the partition silently loses them. Such a file spans two partitions and is now an unknown partition (error row naming the column). A transform that maps every value to null (void) is unaffected: nulls and values land in the same partition. Test and fix together: on the previous commit testPartitionColumnWithNullsAndValuesIsAnUnknownPartition fails with the file registered and no error row. * fix boundadjustment * comments * missing file * comments * trigger tests * move utils, tests * fixes * traverse from newest schema

Addfiles partition bounds fixes (#40252)

claudevdm added 7 commits September 23, 2026 15:16

fix boundadjustment

1a120b0

github-actions Bot added java io iceberg Iceberg IO connector labels Sep 23, 2026

claudevdm marked this pull request as ready for review September 23, 2026 20:38

claudevdm requested a review from ahmedabu98 September 23, 2026 20:38

ahmedabu98 reviewed Sep 25, 2026

View reviewed changes

claudevdm added 7 commits September 27, 2026 19:26

comments

6e9b526

missing file

763ccfc

comments

7e1d9f9

trigger tests

35e2cfe

move utils, tests

438e35c

fixes

29ec94a

traverse from newest schema

d876b7f

claudevdm requested a review from ahmedabu98 September 28, 2026 20:48

ahmedabu98 approved these changes Sep 29, 2026

View reviewed changes

Merge branch 'master' into addfiles-partition-bounds-fixes

9b13a72

claudevdm merged commit bffb545 into apache:master Sep 29, 2026
20 of 21 checks passed

Amar3tto added a commit that referenced this pull request Sep 30, 2026

Merge pull request #40337 from claudevdm/cp-40252

5cd677e

Addfiles partition bounds fixes (#40252)

Navigation Menu

Repository navigation

Addfiles partition bounds fixes - #40252

Addfiles partition bounds fixes#40252
claudevdm merged 15 commits into
apache:masterfrom
claudevdm:addfiles-partition-bounds-fixes

claudevdm commented Sep 23, 2026 •

edited by ahmedabu98

Loading

Uh oh!

claudevdm commented Sep 23, 2026

Uh oh!

github-actions Bot commented Sep 23, 2026

Uh oh!

ahmedabu98 Sep 25, 2026

Uh oh!

claudevdm Sep 28, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Sep 28, 2026

Uh oh!

ahmedabu98 left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

claudevdm commented Sep 23, 2026 • edited by ahmedabu98 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes for users

GitHub Actions Tests Status (on master branch)

Uh oh!

claudevdm commented Sep 23, 2026

Uh oh!

github-actions Bot commented Sep 23, 2026

Uh oh!

ahmedabu98 Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

claudevdm Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Sep 28, 2026

Codecov Report

Uh oh!

ahmedabu98 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

claudevdm commented Sep 23, 2026 •

edited by ahmedabu98

Loading