deniskuzZ commented on code in PR #6789:
URL: https://github.com/apache/hive/pull/6789#discussion_r4247525890
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveTableUtil.java:
##########
@@ -259,6 +262,57 @@ public static Table deserializeTable(Configuration config,
String name) {
return table;
}
+ /**
+ * Resolves the Iceberg {@link Table} for split generation ({@code
IcebergInputFormat#getSplits}).
+ *
+ * <p>Serialized tables ({@link SerializableTable}) only carry a metadata
snapshot and produce
+ * {@link org.apache.iceberg.DataTableScan} (client-side manifest planning).
When REST catalog
+ * server-side scan planning is enabled, reload the live table from the
catalog so
+ * {@code table.newScan()} returns {@link
org.apache.iceberg.rest.RESTTableScan} and issues
+ * {@code POST /plan} on the REST server.
+ *
+ * <p>Intra-transaction read-after-write ({@link
InputFormatConfig#TABLE_METADATA_LOCATION}) still
+ * uses the deserialized snapshot so uncommitted metadata is visible.
+ */
+ public static Table resolveTableForScanPlanning(Configuration conf, String
tableIdentifier) {
+ if (shouldReloadForServerSideScanPlanning(conf)) {
Review Comment:
1. The query can read a different snapshot than the one it compiled against.
HS2 compiles against snapshot N and serializes that table into the job conf.
In server mode, split generation runs later in the Tez AM and calls
Catalogs.loadTable, which returns whatever is current then (N+k)
The TABLE_METADATA_LOCATION guard covers read-after-write inside a
transaction, but not this drift.
The fix is cheap: when no explicit snapshot, ref, or as-of time is set, pin
the REST scan to `serializedTable.currentSnapshot().snapshotId()` and project
the serialized schema. The live table is then used only to send POST /plan.
2. Catalog properties, including secrets, are copied into shipped job
properties.
`propagateCatalogPropertiesToJob` copies every iceberg.catalog.<name>.*
entry. That includes credential, token, and OAuth client secrets if they are
configured. In overlayTableProperties they go into the TableDesc job-properties
map, which is serialized into the plan and job.xml.
Please add test validating that credential / token keys stay out of the
propagated job properties.
3. Each reload leaks a catalog and its HTTP client in a long-lived AM.
Here it runs in the Tez AM, which is reused across queries with session
pooling and LLAP. With OAuth2, the REST session can also own a token-refresh
executor. One unclosed RESTCatalog per scan per query adds up to leaked threads
and connections.
getSplits already materializes planTasks() into a list, so the catalog can
be closed right after planning (try-with-resources around load, plan, and
materialize). The other option is a bounded per-catalog cache in the AM.
Snapshot pinning and secret filtering are blockers, and catalog lifecycle
should at least be fixed for the AM path.
suggested shape:
````
Table snapshotView = deserializeTable(conf, id); // compile-time
truth (schema, specs, snapshot)
if (serverPlanningRequested(conf) && snapshotView != null &&
!intraTxn(conf)) {
try (RestPlanningSession s = RestPlanningSession.open(conf, catalogName))
{ // closes catalog/HTTP client
TableScan scan = s.load(id).newScan()
.useSnapshot(pinnedSnapshotId(conf, snapshotView))
.project(snapshotView.schema());
// apply filters/options as today, then materialize planTasks() before
close
}
}
```
--
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]
