Zoltan Borok-Nagy has posted comments on this change. (
http://gerrit.cloudera.org:8080/24968 )
Change subject: IMPALA-15447: Iceberg scan telemetry (Stream A + B)
......................................................................
Patch Set 4:
(18 comments)
Thanks for working on this!
I'd suggest splitting this into three changes:
(a) a configurable MetricsReporter composed with the existing
InMemoryMetricsReporter, plus query-id in the report metadata.
This part is small and looks reasonable.
(b) synthesized reports on the fast path
(c) query-shape reporting
(c) probably needs design discussion first, please publish a design doc to
[email protected]
http://gerrit.cloudera.org:8080/#/c/24968/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/24968/4//COMMIT_MSG@13
PS4, Line 13: rter
nit: our style guide limits lines of the commit message body to 72 chars.
http://gerrit.cloudera.org:8080/#/c/24968/4//COMMIT_MSG@31
PS4, Line 31:
Please add a "Testing:" section, per the usual Impala commit message
convention, describing how this was verified. Also, "Stream A" and "Stream B"
come from an external design doc. It would be clearer to describe them in terms
of what they do (scan report forwarding, query shape reporting).
http://gerrit.cloudera.org:8080/#/c/24968/4/fe/src/main/java/org/apache/impala/planner/IcebergQueryShapeCollector.java
File fe/src/main/java/org/apache/impala/planner/IcebergQueryShapeCollector.java:
http://gerrit.cloudera.org:8080/#/c/24968/4/fe/src/main/java/org/apache/impala/planner/IcebergQueryShapeCollector.java@45
PS4, Line 45: production_query_engine_reporting.md
This refers to production_query_engine_reporting.md WI5, which isn't part
of the project and isn't visible to reviewers. The same kind of reference
appears in IcebergScanPlanner.java (lines 187, 257, 1418), Frontend.java
(line 2140) and IcebergUtil.java (lines 670-671). Please remove these, and
the Cloudera Manager references, and move the design rationale to the JIRA.
http://gerrit.cloudera.org:8080/#/c/24968/4/fe/src/main/java/org/apache/impala/planner/IcebergQueryShapeCollector.java@73
PS4, Line 73: "com.acme.iceberg.telemetry.QueryShapeReporter"
A hardcoded third-party class name can't go into Apache Impala. Nobody
else can implement this contract without shipping a class with this exact
FQCN and a matching report(Map, String, String, List) signature.
Impala already has a pluggable mechanism for this: QueryEventHook,
configured through query_event_hook_classes, with hooks run on a separate
executor. Could query-shape reporting be built as a QueryEventHook,
possibly by extending the hook context with the join and sort info it
needs? That would give a public interface, keep it off the planning
thread, and remove the need for reflection.
http://gerrit.cloudera.org:8080/#/c/24968/4/fe/src/main/java/org/apache/impala/planner/IcebergQueryShapeCollector.java@115
PS4, Line 115: report.invoke(null, buildConfig(), ENGINE, queryId,
tables);
This call runs synchronously on the planning thread, and its Javadoc says
it POSTs. Whether it blocks depends entirely on the external
implementation, so the "never blocks query planning" claim in the commit
message isn't guaranteed by this code. A slow or hung collector would add
latency to every query. If this stays inline, the work should be handed off
to a bounded executor; the QueryEventHook route would handle that already.
http://gerrit.cloudera.org:8080/#/c/24968/4/fe/src/main/java/org/apache/impala/planner/IcebergQueryShapeCollector.java@132
PS4, Line 132: if (node instanceof SortNode) {
This treats every SortNode as an ORDER BY, which also includes analytic
function sorts and partial sorts added for clustered or sorted INSERTs.
Those don't reflect user ORDER BY clauses and will skew any clustering
recommendations built from this data. Please filter on the sort type, e.g.
only TOTAL and TOPN, and add a test with an analytic function to cover it.
http://gerrit.cloudera.org:8080/#/c/24968/4/fe/src/main/java/org/apache/impala/planner/IcebergQueryShapeCollector.java@174
PS4, Line 174: boolean isAsc = asc != null && i < asc.size() &&
Boolean.TRUE.equals(asc.get(i));
If isAscOrder is null or shorter than the sort exprs list, this reports the
column as "desc". Reporting null or "unknown" would be more accurate than
guessing a direction.
http://gerrit.cloudera.org:8080/#/c/24968/4/fe/src/main/java/org/apache/impala/planner/IcebergQueryShapeCollector.java@194
PS4, Line 194: SlotDescriptor slot = expr.findSrcScanSlot();
Have you verified that findSrcScanSlot() resolves sort expressions back to the
scan tuple? Please add a planner test asserting ORDER BY columns on an Iceberg
table are actually reported.
http://gerrit.cloudera.org:8080/#/c/24968/4/fe/src/main/java/org/apache/impala/planner/IcebergQueryShapeCollector.java@233
PS4, Line 233: if (!initialized_) {
Nit: this double-checked locking is correct, but an initialization-on-demand
holder class would be simpler. The same pattern is duplicated in
IcebergScanPlanner.catalogReporter().
http://gerrit.cloudera.org:8080/#/c/24968/4/fe/src/main/java/org/apache/impala/planner/IcebergQueryShapeCollector.java@265
PS4, Line 265: private static Map<String, String> buildConfig() {
buildConfig() iterates over all of System.getProperties() on every query. The
result can't change after startup, so it could be computed once alongside the
memoized Method. The same filtering logic is also duplicated in
IcebergScanPlanner.loadCatalogReporter() (line 220). More generally, I'd prefer
these settings come from backend flags via BackendConfig rather than -D system
properties, for consistency with how the rest of Impala is configured.
http://gerrit.cloudera.org:8080/#/c/24968/4/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java
File fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java:
http://gerrit.cloudera.org:8080/#/c/24968/4/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@252
PS4, Line 252: return MetricsReporters.combine(metricsReporter_, side);
It's true that MetricsReporters.combine() isolates exceptions from each
reporter. But reporting still happens synchronously on the planning thread, and
the comment's "non-blocking" guarantee relies on TelemetryMetricsReporter,
which isn't part of Impala. Please either document that configured reporters
must not block, or hand reports off to an executor. The same applies to
side.report() at line 1468.
http://gerrit.cloudera.org:8080/#/c/24968/4/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@328
PS4, Line 328: synthesizeAndReportScanReport();
I'd suggest moving fast-path report synthesis into its own change. It creates
ScanReports that Iceberg itself never produced, and I think that deserves a
separate discussion (see the comments below). It also needs its own tests:
exactly one report emitted when telemetry is enabled, none when it's disabled,
and none for metadata-table scans.
http://gerrit.cloudera.org:8080/#/c/24968/4/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@1438
PS4, Line 1438: long deleteFiles = positionDeleteFiles_.size() +
dataFileToDV_.size()
ScanMetrics also has separate counters (positionalDeleteFiles(),
equalityDeleteFiles(), dvs()). Filling in those with distinct counts would be
more accurate than putting everything into resultDeleteFiles().
http://gerrit.cloudera.org:8080/#/c/24968/4/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@1454
PS4, Line 1454: // totalPlanningDuration left null -> no Iceberg planning
occurred.
The Javadoc calls this report "faithful," but it differs from a real one:
totalPlanningDuration is null (the Javadoc notes that this NPEs in
Frontend.fillProfileNodeWithIcebergScanMetrics, and other consumers could hit
the same thing), and skippedDataFiles=0 means "pruning wasn't attempted" rather
than "nothing was pruned." Please add a metadata entry such as
"synthesized"="true" so downstream consumers can tell these reports apart from
ones produced by Iceberg.
http://gerrit.cloudera.org:8080/#/c/24968/4/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@1460
PS4, Line 1460: .schemaId(getIceTable().getIcebergSchema().schemaId())
getIcebergSchema() returns the table's current schema. For a time-travel query,
snapshotId_ can point to a snapshot that used a different schema, so the report
would pair that snapshot with the wrong schemaId. Please use the schema id of
the scanned snapshot instead.
http://gerrit.cloudera.org:8080/#/c/24968/4/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@1461
PS4, Line 1461: .filter(buildResidualFilter()) // REAL
predicate tree -> {col,op}
This sends the raw predicate, literals included, to an external collector.
Iceberg's SnapshotScan runs the filter through ExpressionUtil.sanitize() before
building its ScanReport, so a real report for email = '[email protected]' wouldn't
contain the address, but this synthesized one would.
That makes the fast path leak data the normal path doesn't. Please wrap this in
ExpressionUtil.sanitize(), which is already imported in this file.
http://gerrit.cloudera.org:8080/#/c/24968/4/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/24968/4/fe/src/main/java/org/apache/impala/service/Frontend.java@2141
PS4, Line 2141: IcebergQueryShapeCollector.collectAndPost(planRoots,
queryCtx);
This runs for every planned statement, including EXPLAIN and queries that later
fail admission or get cancelled. Is that intended? It also only covers this
planning path. Hooking into query completion (QueryEventHook) would report only
queries that actually ran, and would keep this work off the critical path.
http://gerrit.cloudera.org:8080/#/c/24968/4/fe/src/main/java/org/apache/impala/util/IcebergUtil.java
File fe/src/main/java/org/apache/impala/util/IcebergUtil.java:
http://gerrit.cloudera.org:8080/#/c/24968/4/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@693
PS4, Line 693: if (reportMetadata != null) {
Two concerns with passing report metadata through scan.option():
1. The correlation feature depends on Iceberg copying scan options into
ScanReport.metadata(), which is internal behavior of the Iceberg
version we're on. Please add a test that asserts query-id actually
shows up in the InMemoryMetricsReporter's report, so an Iceberg upgrade
can't silently break it.
2. It puts keys that aren't read options into the scan's read-options
namespace, where they could collide with future Iceberg options.
Minor: since the Javadoc says metadata is only meaningful with a reporter, this
block could go inside the metricsReporter != null branch above.
--
To view, visit http://gerrit.cloudera.org:8080/24968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a290d79ec467a663af32efcce4bcdfebd7d9287
Gerrit-Change-Number: 24968
Gerrit-PatchSet: 4
Gerrit-Owner: Anonymous Coward <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]>
Gerrit-Comment-Date: Tue, 29 Sep 2026 17:49:46 +0000
Gerrit-HasComments: Yes
