Skip to content

Spark: Fix flaky ordering in Spark TestSelect #14956

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
singhpk234 merged 3 commits into from
Jan 3, 2026
Merged

Spark: Fix flaky ordering in Spark TestSelect #14956

singhpk234 merged 3 commits into from
Jan 3, 2026

Conversation

@huaxingao
Copy link
Contributor

@huaxingao huaxingao commented Jan 3, 2026

Fix flaky ordering in Spark TestSelect.testSelectWithSpecifiedTargetSplitSize()

TestRemoteScanPlanning > testSelectWithSpecifiedTargetSplitSize() > catalogName = testrest, implementation = org.apache.iceberg.spark.SparkCatalog, config = {type=rest, cache-enabled=false, uri=http://localhost:36105/, rest-scan-planning-enabled=true}, binaryTableName = testrest.default.binary_table FAILED
    org.opentest4j.AssertionFailedError: [Should return all expected rows: row 1 contents should match] 
    expected: 1L
     but was: 2L
        at app//org.apache.iceberg.spark.SparkTestHelperBase.assertEquals(SparkTestHelperBase.java:91)
        at app//org.apache.iceberg.spark.SparkTestHelperBase.assertEquals(SparkTestHelperBase.java:67)
        at app//org.apache.iceberg.spark.sql.TestSelect.testSelectWithSpecifiedTargetSplitSize(TestSelect.java:124)

Also fix a few other places that could cause ordering problem.

@github-actions github-actions bot added the spark label Jan 3, 2026
singhpk234
singhpk234 reviewed Jan 3, 2026
View reviewed changes
Copy link
Contributor

@singhpk234 singhpk234 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Mostly LGTM, seems like we might have missed these in earlier fix

should we port this to 4.0 too ?

spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/sql/TestSelect.java Outdated
assertThat(sql("SELECT * FROM %s LIMIT 3", tableName)).containsExactly(first, second, third);
// Note: without ORDER BY, the specific rows returned are not deterministic, especially when
// remote scan planning or split planning changes the physical scan order. This test only
// asserts that the LIMIT is enforced.
Copy link
Contributor

@singhpk234 singhpk234 Jan 3, 2026

Choose a reason for hiding this comment

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

i would still recommend checking the records to make sure same record is not returned twice ?

Copy link
Contributor Author

@huaxingao huaxingao Jan 3, 2026

Choose a reason for hiding this comment

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

Make sense. I change to order by.

singhpk234
singhpk234 approved these changes Jan 3, 2026
View reviewed changes
Copy link
Contributor

@singhpk234 singhpk234 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @huaxingao !

@singhpk234 singhpk234 merged commit e131329 into apache:main Jan 3, 2026
31 checks passed
@singhpk234 singhpk234 mentioned this pull request Jan 5, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@singhpk234 singhpk234 singhpk234 approved these changes

Assignees

No one assigned

Labels

spark

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@huaxingao @singhpk234