You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "aokolnychyi (via GitHub)" <gi...@apache.org> on 2026/02/10 00:51:16 UTC
[PR] Spark 4.1: Simplify description and toString in scans [iceberg]
aokolnychyi opened a new pull request, #15281:
URL: https://github.com/apache/iceberg/pull/15281
This PR contains a subset of changes for #15240 that moves around some scan objects.
A few notable changes:
- Standardize on `description()` that is actually used in Spark plans. No need for almost identical unused `toString()`.
- No need to include the schema as it is already supplied by Spark. [See](https://github.com/apache/spark/blob/3cf6e6b1020ec8a1462fdc036c3b5f7f7a6cc50e/sql/catalyst/src/main/java/org/apache/spark/sql/connector/read/Scan.java#L63) Javadoc.
- No need to include useless info like case sensitivity.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark 4.1: Simplify description and toString in scans [iceberg]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #15281:
URL: https://github.com/apache/iceberg/pull/15281#issuecomment-3877945871
@aokolnychyi, would you mind providing some concrete examples (screenshots) of how this change affects Spark UI / EXPLAIN outputs? Displaying seems not properly considered and documented in the Spark DSv2 API, we internally make some hacky changes to make the UI display pretty ...
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark 4.1: Simplify description and toString in scans [iceberg]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #15281:
URL: https://github.com/apache/iceberg/pull/15281#discussion_r2785353967
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/source/SparkScan.java:
##########
@@ -257,15 +261,8 @@ private long totalRecords(Snapshot snapshot) {
}
@Override
- public String description() {
- String groupingKeyFieldNamesAsString =
- groupingKeyType().fields().stream()
- .map(Types.NestedField::name)
- .collect(Collectors.joining(", "));
-
- return String.format(
- "%s (branch=%s) [filters=%s, groupedBy=%s]",
- table(), branch(), Spark3Util.describe(filterExpressions), groupingKeyFieldNamesAsString);
+ public String toString() {
+ return description();
}
@Override
Review Comment:
This change makes `toString()` delegate to `description()`, but `Scan.description()` is commonly implemented as a default method that delegates to `toString()` in Spark (per the Scan Javadoc pattern). If `SparkScan` (or any subclass) relies on the default `description()`, this can create infinite recursion (`description() -> toString() -> description() ...`) and a stack overflow. Fix by ensuring `SparkScan` provides a concrete `description()` implementation (and have `toString()` call that), or by removing the `toString()` override here and letting `description()` remain the single source of truth.
```suggestion
```
##########
spark/v4.1/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMerge.java:
##########
@@ -3014,7 +3014,7 @@ private void checkJoinAndFilterConditions(String query, String join, String iceb
assertThat(planAsString)
.as("Pushed filters must match")
- .contains("[filters=" + icebergFilters + ",");
+ .contains("filters=" + icebergFilters + ",");
Review Comment:
Same issue as above: `runtimeFilters=` includes `filters=` as a substring, so this can accidentally match the wrong section of the plan string. Tighten the assertion to include an unambiguous delimiter (e.g., `\", filters=\"`) or context around the expected field.
```suggestion
.contains(", filters=" + icebergFilters + ",");
```
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/sql/TestFilterPushDown.java:
##########
@@ -676,7 +676,7 @@ private void checkFilters(
assertThat(planAsString)
.as("Pushed filters must match")
- .contains("[filters=" + icebergFilters + ",");
+ .contains("filters=" + icebergFilters + ",");
Review Comment:
This assertion can produce false positives because `runtimeFilters=` contains the substring `filters=`. If `icebergFilters` happens to match the runtime filter string, the test could pass even if the main `filters=` section is missing/incorrect. Make the match more specific (e.g., include a delimiter like `\", filters=\"` or match the full scan prefix such as `\"IcebergScan(\"` + `\"filters=...\"`) so it cannot accidentally match `runtimeFilters=`.
```suggestion
.contains("filters=" + icebergFilters + ", runtimeFilters=");
```
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/source/SparkScan.java:
##########
@@ -153,6 +153,10 @@ protected List<Expression> filterExpressions() {
return filterExpressions;
}
+ protected String filtersDesc() {
+ return Spark3Util.describe(filterExpressions);
Review Comment:
Using a human-readable description string as a proxy for filter identity is risky when it is later used by `equals()`/`hashCode()` in subclasses (as introduced in this PR). Different filter expression trees can potentially serialize to the same description string (lossy formatting, reordered terms, elided parentheses), which can make `equals()` return true for non-equal scans and break hash-based collections/caches. Prefer comparing the actual expression objects (or a canonical, non-lossy representation) in `equals()`/`hashCode()` rather than `Spark3Util.describe(...)` output.
```suggestion
return filterExpressions.stream()
.map(Expression::toString)
.collect(Collectors.joining(" AND "));
```
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/source/TestSparkScan.java:
##########
@@ -1022,6 +1023,50 @@ public void testPartitionedOr() throws Exception {
assertThat(scan.planInputPartitions()).hasSize(4);
}
+ @TestTemplate
+ public void testBatchQueryScanDescription() throws Exception {
+ createPartitionedTable(spark, tableName, "data");
+ SparkScanBuilder builder = scanBuilder();
+
+ withSQLConf(
+ ImmutableMap.of(SparkSQLProperties.PRESERVE_DATA_GROUPING, "true"),
+ () -> {
+ Predicate predicate1 = new Predicate("=", expressions(fieldRef("id"), intLit(1)));
+ Predicate predicate2 = new Predicate(">", expressions(fieldRef("id"), intLit(0)));
+ pushFilters(builder, predicate1, predicate2);
+
+ Scan scan = builder.build();
+ String description = scan.description();
+
+ assertThat(description).contains("IcebergScan");
+ assertThat(description).contains(tableName);
+ assertThat(description).contains("filters=id = 1, id > 0");
+ assertThat(description).contains("groupedBy=data");
+ });
+ }
+
+ @TestTemplate
+ public void testCopyOnWriteScanDescription() throws Exception {
+ createPartitionedTable(spark, tableName, "data");
+ SparkScanBuilder builder = scanBuilder();
+
+ withSQLConf(
+ ImmutableMap.of(SparkSQLProperties.PRESERVE_DATA_GROUPING, "true"),
+ () -> {
+ Predicate predicate1 = new Predicate("=", expressions(fieldRef("id"), intLit(2)));
+ Predicate predicate2 = new Predicate("<", expressions(fieldRef("id"), intLit(10)));
+ pushFilters(builder, predicate1, predicate2);
+
+ Scan scan = builder.buildCopyOnWriteScan();
+ String description = scan.description();
+
+ assertThat(description).contains("IcebergCopyOnWriteScan");
+ assertThat(description).contains(tableName);
+ assertThat(description).contains("filters=id = 2, id < 10");
Review Comment:
These assertions bake in the exact formatting and ordering of the rendered filter description (e.g., `\"id = 1, id > 0\"`). If `Spark3Util.describe(...)` changes formatting (commas vs AND, spacing, predicate order), the tests will fail even though behavior is correct. Consider asserting each predicate independently (e.g., contains `\"id = 1\"` and contains `\"id > 0\"`) and separately asserting the `filters=` label, rather than asserting the full combined string in a single contains check.
```suggestion
assertThat(description).contains("filters=");
assertThat(description).contains("id = 2");
assertThat(description).contains("id < 10");
```
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/source/SparkPartitioningAwareScan.java:
##########
@@ -250,4 +250,10 @@ private StructLikeSet collectGroupingKeys(Iterable<ScanTaskGroup<T>> taskGroupIt
return keys;
}
+
+ protected String groupingKeyDesc() {
+ return groupingKeyType().fields().stream()
+ .map(org.apache.iceberg.types.Types.NestedField::name)
+ .collect(Collectors.joining(", "));
+ }
Review Comment:
Using a fully-qualified class reference inside the stream (`org.apache.iceberg.types.Types.NestedField::name`) reduces readability and is inconsistent with nearby code that already refers to `Types`. Prefer adding/using an import and referencing `Types.NestedField::name` instead.
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/source/TestSparkScan.java:
##########
@@ -1022,6 +1023,50 @@ public void testPartitionedOr() throws Exception {
assertThat(scan.planInputPartitions()).hasSize(4);
}
+ @TestTemplate
+ public void testBatchQueryScanDescription() throws Exception {
+ createPartitionedTable(spark, tableName, "data");
+ SparkScanBuilder builder = scanBuilder();
+
+ withSQLConf(
+ ImmutableMap.of(SparkSQLProperties.PRESERVE_DATA_GROUPING, "true"),
+ () -> {
+ Predicate predicate1 = new Predicate("=", expressions(fieldRef("id"), intLit(1)));
+ Predicate predicate2 = new Predicate(">", expressions(fieldRef("id"), intLit(0)));
+ pushFilters(builder, predicate1, predicate2);
+
+ Scan scan = builder.build();
+ String description = scan.description();
+
+ assertThat(description).contains("IcebergScan");
+ assertThat(description).contains(tableName);
+ assertThat(description).contains("filters=id = 1, id > 0");
+ assertThat(description).contains("groupedBy=data");
Review Comment:
These assertions bake in the exact formatting and ordering of the rendered filter description (e.g., `\"id = 1, id > 0\"`). If `Spark3Util.describe(...)` changes formatting (commas vs AND, spacing, predicate order), the tests will fail even though behavior is correct. Consider asserting each predicate independently (e.g., contains `\"id = 1\"` and contains `\"id > 0\"`) and separately asserting the `filters=` label, rather than asserting the full combined string in a single contains check.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark 4.1: Simplify description and toString in scans [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi merged PR #15281:
URL: https://github.com/apache/iceberg/pull/15281
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark 4.1: Simplify description and toString in scans [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #15281:
URL: https://github.com/apache/iceberg/pull/15281#issuecomment-3878312812
@pan3793, I am going to merge this one to rebase my other PR. However, I will check in for any additional feedback you may have. Let me know what you think and whether there are gaps we should fix either in Iceberg or in Spark.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark 4.1: Simplify description and toString in scans [iceberg]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #15281:
URL: https://github.com/apache/iceberg/pull/15281#issuecomment-4384714111
for spark ui display, cross link https://github.com/apache/spark/pull/55510#discussion_r3135817324
@aokolnychyi would be great if you could take a look.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark 4.1: Simplify description and toString in scans [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #15281:
URL: https://github.com/apache/iceberg/pull/15281#issuecomment-3878284441
@pan3793, if I recall correctly, `Scan$description` is only used in scan exec nodes. This PR removes the projection schema per Javadoc instructions in Spark and adds the scan class to the name (I am planning to add even more scan types later so it is helpful to see with which scan we are dealing with). I debated whether I should preserve the runtime filters field but decided to keep it as is for now, even though it somewhat duplicates what Spark includes.
```
== Physical Plan ==
*(1) ColumnarToRow
+- BatchScan testhive.default.table[id#39, dep#40] IcebergScan(table=testhive.default.table, branch=null, filters=, runtimeFilters=, groupedBy=) RuntimeFilters: []
```
```
== Physical Plan ==
ReplaceData IcebergWrite(table=testhive.default.table, format=ORC)
+- AdaptiveSparkPlan isFinalPlan=false
+- MergeRowsExec[__row_operation#55, id#56, dep#57, _file#58]
+- SortMergeJoin [id#39], [id#33], FullOuter
:- Sort [id#39 ASC NULLS FIRST], false, 0
: +- Exchange hashpartitioning(id#39, 4), ENSURE_REQUIREMENTS, [plan_id=91]
: +- Project [id#39, dep#40, _file#48, true AS __row_from_target#51, monotonically_increasing_id() AS __row_id#52L]
: +- BatchScan testhive.default.table[id#39, dep#40, _file#48] IcebergCopyOnWriteScan(table=testhive.default.table, filters=true, groupedBy=) RuntimeFilters: []
+- Sort [id#33 ASC NULLS FIRST], false, 0
+- Exchange hashpartitioning(id#33, 4), ENSURE_REQUIREMENTS, [plan_id=92]
+- Project [id#33, dep#34, true AS __row_from_source#53]
+- Scan ExistingRDD[id#33,dep#34]
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org