Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/24753
to look at the new patch set (#2).
Change subject: IMPALA-15307: Fixes from reading the Substrait export series
whole
......................................................................
IMPALA-15307: Fixes from reading the Substrait export series whole
Reading the series as one body of code, rather than one change at a
time, turns up four things that each change on its own did not.
Operand narrowing reached DECIMAL. Calcite's EXACT_TYPES is the integer
types plus DECIMAL, so a decimal operand of substr or round was cast to
an integer and everything after the point was dropped, silently.
Narrowing covers the integer types only now, and a decimal operand is
left as it is so the call is refused rather than answered wrongly.
Folding a constant interval multiply dereferenced a literal's value
without checking it. RexLiteral.getValueAs returns null for a NULL
literal, so a plan carrying one failed with a NullPointerException
reported as an internal fault instead of being refused with a reason.
The statistics visitor called RelMetadataQuery outside the guard. A
metadata handler can refuse a relation shape or find a cycle, and a plan
that exported before it carried estimates would then be rejected. Worse,
the exporter classified by exception class, so the caller was told the
fault was an unmapped function and handed an operator inventory for it.
The metadata calls are inside the guard now, and a lost estimate costs
an estimate rather than an export. While there, the hint is built from
what the relation already carries, so an alias or output names a later
Isthmus attaches are not replaced by a hint holding statistics alone.
The provider held one visitor. ConverterProvider is memoized for the
process and a SubstraitRelVisitor carries state for the conversion it is
running, so every export would have answered with the resolver of
whichever export built it. A fresh visitor per call, as the base class
gives.
Three tidyings from the same reading. Three public accessors on the
mapping table were documented as being there for a test that checks the
table rather than a plan; no such test was ever written and nothing
calls them. The execution behaviour was assembled by hand when
ConverterProvider already builds exactly it and hands it over, so the
constant was copied out of the library that owns it. And an import had
outlived its use.
The AssertionError and classification fixes that came out of the same
reading are in IMPALA-15296, where the code they touch lives.
Testing, in a local cluster on Linux, the five classes in one run,
42 tests, 0 failures, 0 errors:
- SubstraitRoundTripTest gains a test that two calls for a visitor give
two visitors
- SubstraitTpcdsExportTest still reports 102 of 110, so restricting the
narrowing to integers costs no query
- SubstraitPlanExporterTest: 22 tests, CalciteOptimizerTest 11,
CalciteSubstraitCompatibilityTest 4
- The same run with the mapping table's accessors gone, so nothing was
calling them
- git diff --check
Change-Id: I4bb6759e226c75a1ca967ba42f82dd947070d93a
Assisted-by: Claude Opus 5 (Anthropic)
---
M
java/calcite-planner/src/test/java/org/apache/impala/calcite/service/SubstraitOperatorMappings.java
M
java/calcite-planner/src/test/java/org/apache/impala/calcite/service/SubstraitPlanExporter.java
M
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
5 files changed, 58 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/24753/2
--
To view, visit http://gerrit.cloudera.org:8080/24753
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4bb6759e226c75a1ca967ba42f82dd947070d93a
Gerrit-Change-Number: 24753
Gerrit-PatchSet: 2
Gerrit-Owner: Aleksandr Efimov <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
