Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/24742
to look at the new patch set (#3).
Change subject: IMPALA-15297: Map Impala operators onto Substrait functions
......................................................................
IMPALA-15297: Map Impala operators onto Substrait functions
Twenty of the twenty-four shapes the direct harness covers export end
to end. The rest stop on an operator Isthmus cannot resolve, and for
most of them the reason is not that Substrait lacks the function.
Isthmus resolves a call by looking the operator up in a map keyed on
the operator itself, and Calcite compares operators by class, name and
kind. Impala substitutes its own classes for a good number of
functions, so an operator that means exactly what Substrait's does
still misses. Isthmus takes additional signatures for this: they say
which Substrait function an Impala operator stands for and leave the
operand matching alone.
Add them for the operators that have a counterpart. For scalars that is
coalesce, concat, length, and floor and ceil with their d-prefixed
spellings. Impala reaches those by three routes, the
USE_IMPALA_OPERATOR set in ImpalaOperatorTable, builtins Calcite has no
operator for, and the replacements in ImpalaCustomOperatorTable, and
all three resolve by name, so one table covers them.
For windows it is lag, first_value and last_value. Isthmus resolves
those through a converter of its own that first tries to normalise the
aggregate by SqlKind; Impala's window operators carry SqlKind.OTHER, so
normalisation passes them through and the lookup lands on the operator
itself, exactly as for scalars. lead needs nothing, because
USE_IMPALA_OPERATOR lists lag but not lead and lead therefore already
arrives as Calcite's own operator.
Together that takes the corpus from twenty to twenty-three.
This is a list of what Isthmus has been taught, not the list of what it
refuses that IMPALA-15296 deliberately does not keep. Rejection stays
Isthmus's decision; every entry here is one a test covers.
What is left needs a different mechanism rather than another name, and
the table says so where a reader would otherwise read an omission.
Substrait models a conditional as the IfThen expression rather than as
a function, so IF has no name to map to, and it is the one corpus shape
still not exporting. Substrait's round takes its digit count as i32
where Impala types the literal as i8, so the name lines up but the
operands do not. Substrait has no truncate at all. NDV is left out on
other grounds: approx_count_distinct is the obvious counterpart, but
that is a rename carrying a claim about the estimator rather than a
matter of operator class, and it wants deciding on its own.
Testing, in a local cluster on Linux, the three classes in one run:
- SubstraitPlanExporterTest: 12 tests, 0 failures, 0 errors
- CalciteOptimizerTest and CalciteSubstraitCompatibilityTest, neither of
which this patch touches: 11 and 4 tests, 0 failures
- Pointing the exporter back at the stock ConverterProvider fails the
mapping test, so that assertion is not vacuous
- A probe over the harness corpus reports 23 of 24 with this patch and
20 of 24 without it
- git diff --check
Change-Id: I1824fa32d7cc294ee8e54c715bb4b40d37a152dd
Assisted-by: Claude Opus 5 (Anthropic)
---
A
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/SubstraitPlanExporterTest.java
3 files changed, 216 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/24742/3
--
To view, visit http://gerrit.cloudera.org:8080/24742
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1824fa32d7cc294ee8e54c715bb4b40d37a152dd
Gerrit-Change-Number: 24742
Gerrit-PatchSet: 3
Gerrit-Owner: Aleksandr Efimov <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
