Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/24752
to look at the new patch set (#4).
Change subject: IMPALA-15306: Carry Calcite's estimates into the Substrait plan
......................................................................
IMPALA-15306: Carry Calcite's estimates into the Substrait plan
A plan leaves the exporter saying nothing about how much data any
relation is expected to produce. Isthmus derives no statistics of its
own, so a consumer planning or costing the query has to guess. Substrait
has a place for this on every relation, and Calcite has already worked
the numbers out by the time the plan reaches the seam, so the two only
need connecting.
SubstraitRelVisitor.apply is public and every relation goes through it,
so a subclass can attach RelMetadataQuery's row count and average row
size to what it returns.
Nothing is asserted that Substrait does not already treat as
approximate: its own words for the field are "estimated statistics" and
"the estimated row count". That matters here, because Impala's row count
is a real one from COMPUTE STATS when the table has been analysed and an
estimate derived from file sizes when it has not, and CalciteTable hands
both over through the same getRowCount. Writing either into a field
declared as an estimate says nothing untrue; writing them into a field
that promised a fact would.
The estimates are attached to what reaches apply rather than to the
Calcite node they came from. Isthmus drops a projection that only
renames, returning its input instead, and rewrites an aggregate before
the visitor sees it, so binding to the returned relation keeps the
numbers on the relation a consumer will read. The hint is built from
what that relation already carries, so an alias or output names a later
Isthmus attaches are not replaced by a hint holding statistics alone.
Losing an estimate costs an estimate and not an export: a metadata
handler can refuse a relation shape or find a cycle, and a relation that
is not Immutables-backed has no setter for a hint.
A visitor is built per call rather than held. The provider is memoized
for the process and a visitor carries state for the conversion it is
running, so one held here would have every export answering with the
resolver of whichever export built it.
This is what needed substrait-java 0.101.0: Rel.withHint arrived there,
and 0.100.0 can read a hint but has no type-agnostic way to set one.
Testing, in a local cluster on Linux, the five classes in one run,
42 tests, 0 failures, 0 errors:
- SubstraitRoundTripTest gains two tests. One checks that the exported
relation carries the numbers Calcite gives for the same node, so a
wrong wiring shows as a mismatch rather than a plausible figure.
Dropping the visitor override fails that one, and only that one, so it
is not vacuous. The other checks that two calls for a visitor give two
visitors; it passes without this patch as well, because the base class
already builds one per call, and is here to catch a future attempt to
hold one rather than to pin anything added now
- SubstraitTpcdsExportTest still reports 95 of 103, so carrying
estimates costs no query
- SubstraitPlanExporterTest 22, CalciteOptimizerTest 11 and
CalciteSubstraitCompatibilityTest 4, none of which this patch touches.
The last asserts on the protobuf a consumer reads, so it is also the
check that adding hints disturbs nothing already there
- git diff --check
Assisted-by: Claude Opus 5 (Anthropic)
Change-Id: Id581d390fe08b7034327c69899c6b1bf492c99c2
---
M
java/calcite-planner/src/test/java/org/apache/impala/calcite/service/SubstraitOperatorMappings.java
A
java/calcite-planner/src/test/java/org/apache/impala/calcite/service/SubstraitPlanStatistics.java
M
java/calcite-planner/src/test/java/org/apache/impala/calcite/service/SubstraitRoundTripTest.java
3 files changed, 144 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/24752/4
--
To view, visit http://gerrit.cloudera.org:8080/24752
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id581d390fe08b7034327c69899c6b1bf492c99c2
Gerrit-Change-Number: 24752
Gerrit-PatchSet: 4
Gerrit-Owner: Aleksandr Efimov <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
