Skip to content

[AURON #2429] Add missing fallback coverage for Iceberg changelog native scan#2430

Open
weimingdiit wants to merge 1 commit into
apache:masterfrom
weimingdiit:test/iceberg-changelog-native-fallback-coverage
Open

[AURON #2429] Add missing fallback coverage for Iceberg changelog native scan#2430
weimingdiit wants to merge 1 commit into
apache:masterfrom
weimingdiit:test/iceberg-changelog-native-fallback-coverage

Conversation

@weimingdiit

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #2429

Rationale for this change
Auron Iceberg native changelog scan is intentionally limited to insert-only changelog tasks with a single supported file format.

Existing tests cover insert-only changelog correctness, changelog metadata columns, and delete changelog fallback. This PR adds coverage for two remaining unsupported boundaries: non-insert changelog operations and mixed file formats.

What changes are included in this PR?
Adds a changelog fallback test for unsupported changelog operations.
Adds a changelog fallback test for mixed Parquet/ORC file formats.
Both tests compare results with Spark execution and verify that NativeIcebergTableScan is not used.
No execution logic is changed.

Are there any user-facing changes?
No user-facing changes. This is a test-only change.

How was this patch tested?
UT.

Was this patch authored or co-authored using generative AI tooling?

  • Yes
  • No

…og native scan

Signed-off-by: weimingdiit <weimingdiit@gmail.com>
|alter table local.db.t_changelog_mixed_formats
|set tblproperties ('write.format.default' = 'orc')
|""".stripMargin)
sql("insert into local.db.t_changelog_mixed_formats values (2, 'orc')")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we assert that the changelog tasks actually contain both Parquet and ORC files before checking fallback? Otherwise, this test may pass because of an unrelated fallback and not exercise the mixed-format branch it intends to cover.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I added a precondition assertion for this test.

The test now inspects the planned changelog tasks before checking fallback and verifies that all changelog tasks are AddedRowsScanTask, and that their data file formats are exactly PARQUET and ORC. This makes sure the test is exercising the intended mixed-format fallback branch, rather than passing due to an unrelated changelog fallback.

|using iceberg
|tblproperties (
| 'format-version' = '2',
| 'write.delete.mode' = 'merge-on-read'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went looking at the CI log for this commit to see which guard actually catches this case, and the delete seems to land on the metadata path rather than merge-on-read: the DELETE FROM commits as StreamingDelete with removedDataFiles=CounterResult{unit=COUNT, value=1}, totalDeleteFiles=CounterResult{unit=COUNT, value=0} and addedPositionalDeleteFiles=null, so no delete file gets written. The two-row insert above landed in two data files (The input RDD has 2 partitions, Committing append with 2 new data files), so id = 1 covers a whole file and Iceberg can drop it by metadata regardless of write.delete.mode.

If that reading is right, the fallback is coming from the task-type check at IcebergScanSupport.scala:266, the same one the existing "falls back when delete changes exist" test hits, and the delete-file check just below at :274 is still waiting for a test. Does that match what you were going for, or were you hoping this one would reach :274?

The test looks like it earns its place either way: it's the only one where added and deleted tasks share a single changelog range, which a guard like addedRowsTasks.isEmpty would get wrong. That just leaves the merge-on-read property as a loose end. Is it worth keeping when it isn't changing the outcome?

On the name, same thread of thought: "unsupported changelog operations" sent me looking for the operation() != INSERT check at :270, but AddedRowsScanTask.operation() in Iceberg 1.10.1 is a default returning INSERT that neither BaseAddedRowsScanTask nor its split subclass overrides, so after the collect at :263 I don't think it can fire. Would something like "falls back when a changelog range mixes inserts and deletes" capture the case better?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that matches what is happening here. This case does not reach the delete-file guard. The fallback comes from the changelog task-type check because the changelog range contains both added-row tasks and non-added-row tasks.

I renamed the test to "iceberg changelog scan falls back when a changelog range mixes inserts and deletes", removed the misleading merge-on-read delete property, and added assertions that the planned changelog tasks contain both AddedRowsScanTask and non-AddedRowsScanTask. That makes the intended coverage explicit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add missing fallback coverage for Iceberg changelog native scan

3 participants