Hello Joe McDonnell, Steve Carlin, Michael Smith, 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 (#11).
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 attaches RelMetadataQuery's row count and
average row size to what it returns.
Nothing is asserted that Substrait does not already treat as
approximate. That matters 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.
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. 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.
The TPC-DS run names any export whose root relation comes out without an
estimate. Attaching them one relation at a time is what makes that worth
asking: the test that checks a number checks one root, and that root is
a projection, so it would still pass with every other shape left bare.
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, 42 tests over five classes, 0
failures:
- SubstraitRoundTripTest gains two tests. One checks that the exported
relation carries the numbers Calcite gives for the same node; dropping
the visitor override fails that one and only that one. The other
checks that two calls for a visitor give two visitors, which passes
without this patch too and is here to catch a future attempt to hold
one
- SubstraitTpcdsExportTest still reports 95 of 103, so carrying
estimates costs no query, and now also reports that every one of those
95 roots carries an estimate. Withholding the visitor fails it with
all 95 named
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
M
java/calcite-planner/src/test/java/org/apache/impala/calcite/service/SubstraitTpcdsExportTest.java
4 files changed, 193 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/24752/11
--
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: 11
Gerrit-Owner: Aleksandr Efimov <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
Gerrit-Reviewer: Joe McDonnell <[email protected]>
Gerrit-Reviewer: Michael Smith <[email protected]>
Gerrit-Reviewer: Steve Carlin <[email protected]>
