huaxingao commented on code in PR #17956:
URL: https://github.com/apache/iceberg/pull/17956#discussion_r3939589658
##########
spark/v4.2/spark/src/main/java/org/apache/iceberg/spark/source/SparkPartitioningAwareScan.java:
##########
@@ -181,6 +193,8 @@ protected synchronized List<T> tasks() {
if (tasks == null) {
try (CloseableIterable<? extends ScanTask> taskIterable =
scan.planFiles()) {
List<T> plannedTasks = Lists.newArrayList();
+ Map<Integer, PartitionPredicateEvaluator> evaluatorsBySpecId =
Maps.newHashMap();
+ int numPlannedTasks = 0;
Review Comment:
nit: `numPlannedTasks` counts tasks before pruning, but `plannedTasks` holds
the ones that survived. Maybe rename it to `numScannedTasks` or
`numCandidateTasks`?
##########
spark/v4.2/spark/src/main/java/org/apache/iceberg/spark/source/BaseSparkScanBuilder.java:
##########
@@ -178,15 +192,15 @@ public Predicate[] pushPredicates(Predicate[] predicates)
{
}
}
- this.filters = expressions;
- this.pushedPredicates = pushablePredicates.toArray(new Predicate[0]);
+ this.filters.addAll(expressions);
Review Comment:
Accumulating is correct here, `SupportsPushDownV2Filters` requires it once
`supportsIterativePushdown()` returns true. But it changes what
`TestFilteredScan`.`testUnpartitionedIDFilters` exercises: that test builds the
`SparkScanBuilder` once at line 204, outside its loop, so iteration i now plans
`id = 0` AND `id = 1` AND … AND `id = i` instead of just `id = i`.
It stays green because Iceberg checks each conjunct separately against the
file bounds [0, 9] and never sees that the conjunction is unsatisfiable, and
the row-level assertion below it goes through a separate read(...) that doesn't
touch the builder. So no coverage is lost outright, but the hasSize(1)
assertion is no longer checking the filter it appears to push, and it now
quietly depends on Iceberg not detecting contradictions.
Could you move the builder construction inside the loop? That matches
`testUnpartitionedCaseInsensitiveIDFilters` just below it:
```
for (int i = 0; i < 10; i += 1) {
SparkScanBuilder builder =
new SparkScanBuilder(spark, TABLES.load(options.get("path")),
options);
pushFilters(builder, EqualTo.apply("id", i));
```
##########
spark/v4.2/spark/src/main/java/org/apache/iceberg/spark/source/SparkPartitioningAwareScan.java:
##########
@@ -229,7 +258,8 @@ protected synchronized List<ScanTaskGroup<T>> taskGroups() {
StructLikeSet plannedGroupingKeys =
collectGroupingKeys(plannedTaskGroups);
LOG.debug(
- "Planned {} task group(s) with {} grouping key type and {} unique
grouping key(s) for table {}",
+ "Planned {} task group(s) with {} grouping key type and {} unique
grouping key(s) for"
+ + " table {}",
Review Comment:
unnecessary change? There are a few other places.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
