jerryshao commented on code in PR #11219:
URL: https://github.com/apache/gravitino/pull/11219#discussion_r3322717984
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java:
##########
@@ -454,62 +454,26 @@ public PlanTableScanResponse planTableScan(
return cachedResponse.get();
}
- List<String> planTasks = new ArrayList<>();
- Map<Integer, PartitionSpec> specsById = new HashMap<>();
- List<DeleteFile> deleteFiles = new ArrayList<>();
+ List<FileScanTask> fileScanTasksList = new ArrayList<>();
try (CloseableIterable<FileScanTask> fileScanTasks =
createFilePlanScanTasks(table, tableIdentifier, scanRequest)) {
for (FileScanTask fileScanTask : fileScanTasks) {
- try {
- String taskString = ScanTaskParser.toJson(fileScanTask);
- planTasks.add(taskString);
-
- int specId = fileScanTask.spec().specId();
- if (!specsById.containsKey(specId)) {
- specsById.put(specId, fileScanTask.spec());
- }
-
- if (!fileScanTask.deletes().isEmpty()) {
- deleteFiles.addAll(fileScanTask.deletes());
- }
- } catch (Exception e) {
- throw new RuntimeException(
- String.format(
- "Failed to serialize scan task for table: %s. Error: %s",
- tableIdentifier, e.getMessage()),
- e);
- }
+ fileScanTasksList.add(fileScanTask);
Review Comment:
**Error context lost during scan task collection**
The old code wrapped `ScanTaskParser.toJson()` per-task in a `try/catch`
that included `tableIdentifier` in the error message. Now that task objects are
collected into a list and serialized later (by the JAX-RS response framework),
any serialization failure will surface as a generic HTTP 500 with no indication
of which table or task caused it.
Consider wrapping the later `buildCompletedPlanTableScanResponse` call in a
catch that adds the `tableIdentifier` context, or logging the identifier before
handing off to the builder.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java:
##########
@@ -528,6 +492,23 @@ public PlanTableScanResponse planTableScan(
}
}
+ /**
+ * Builds a synchronous COMPLETED scan plan response for Iceberg 1.11+ REST
clients only.
+ *
+ * <p>Matches {@link CatalogHandlers#planTableScan}: {@code file-scan-tasks}
plus {@code
+ * specs-by-id} from {@link Table#specs()}. Does not populate legacy {@code
plan-tasks} JSON
+ * strings.
+ */
+ @SuppressWarnings("deprecation")
+ private static PlanTableScanResponse buildCompletedPlanTableScanResponse(
+ Table table, List<FileScanTask> fileScanTasks) {
+ return PlanTableScanResponse.builder()
+ .withPlanStatus(PlanStatus.COMPLETED)
+ .withFileScanTasks(fileScanTasks)
+ .withSpecsById(table.specs())
Review Comment:
**`table.specs()` returns all historical partition specs, not just those
referenced by the returned scan tasks**
The old code built `specsById` incrementally — only partition specs actually
seen in the scanned tasks were included in the response. The new code passes
`table.specs()` which includes every partition spec ever applied to the table,
including ones from long-expired schema evolutions.
For a filtered scan that returns zero tasks (predicate fully prunes the
table), the response now carries all N historical specs over the wire where the
old code sent none. This matches `CatalogHandlers#planTableScan` in Iceberg
1.11, so it is intentional — but worth confirming that the extra wire payload
is acceptable, particularly for tables with many historical spec evolutions.
##########
gradle/libs.versions.toml:
##########
@@ -64,8 +64,10 @@ commons-beanutils = "1.11.0"
commons-configuration1 = "1.6"
commons-dbcp2 = "2.11.0"
caffeine = "2.9.3"
-iceberg = '1.10.1' # used for Gravitino Iceberg catalog and Iceberg REST
service
-iceberg4connector = "1.6.1" # used for compile connectors like Spark, Flink,
etc
+iceberg = '1.11.0'
+iceberg4spark33 = "1.8.1"
Review Comment:
**Spark 3.3 connector (Iceberg 1.8.1) tested against Gravitino's Iceberg
1.11 REST server in deploy mode**
Iceberg dropped Spark 3.3 support after 1.9.x, so `iceberg4spark33 =
"1.8.1"` is the last available version. The `@DisabledIf(isEmbedded)` on the
Spark 3.3 IT classes correctly avoids classloader conflicts in embedded mode,
but in deploy mode a 1.8.1 client will be talking to a 1.11 server.
API surface changes between 1.8.1 and 1.11 include the `file-scan-tasks`
response format for scan planning and the new namespace separator contract —
both introduced by this PR. The Spark 3.3 deploy-mode ITs may pass simply
because they do not exercise those specific endpoints, providing false
confidence. Consider adding an explicit note in the test comment that
deploy-mode Spark 3.3 tests cover only basic CRUD and not scan-planning or
multi-level namespace operations.
--
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]
