Skip to content

Conversation

@wypoon
Copy link
Contributor

@wypoon wypoon commented Mar 8, 2025

This fixes a bug in SparkScan::estimateStatistics(Snapshot).
Table::statisticsFiles() returns a List<StatisticsFile>. We need to get the StatisticsFile with the snapshotId of the Snapshot for use in estimating the statistics.
I modified an existing test so that it fails without the fix and passes with it.

Table::statisticsFiles() returns a List. We need to get the StatisticsFile
with the snapshotId of the Snapshot.
@github-actions github-actions bot added the spark label Mar 8, 2025

Map<String, Long> expectedNDV = Maps.newHashMap();
expectedNDV.put("id", 6L);
withSQLConf(reportColStatsEnabled, () -> checkColStatisticsReported(scan, 6L, expectedNDV));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test is parameterized with three parameters. When run on its own without the fix, from one to three cases will fail. The reason some of the time the correct StatisticsFile appears as the first in the List is that when TableMetadata is built (

statisticsFiles.values().stream().flatMap(List::stream).collect(Collectors.toList()),
), the List is built from a Map and the order of the entries depend on the hashing of the snapshotId (which is random).

@wypoon
Copy link
Contributor Author

wypoon commented Mar 10, 2025

@huaxingao can you please review this?

@wypoon
Copy link
Contributor Author

wypoon commented Mar 20, 2025

@findepi can you please review this simple fix?

@findepi findepi merged commit ff5004e into apache:main Mar 20, 2025
27 checks passed
@wypoon
Copy link
Contributor Author

wypoon commented Mar 20, 2025

Thanks @findepi!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants