Hello Joe McDonnell, Steve Carlin, Michael Smith, Stamatis Zampetakis, Impala
Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/24741
to look at the new patch set (#12).
Change subject: IMPALA-15296: Export the seam plan to Substrait
......................................................................
IMPALA-15296: Export the seam plan to Substrait
Add an exporter next to the IMPALA-15282 compatibility spike. It takes
the plan at the pre-conversion seam and either produces a Substrait plan
or says why there is none. A rejection carries a reason, what Isthmus
said, and the relations or operators the plan contained, so a caller
does not have to re-walk the plan to find out what stopped it. It never
returns a half-converted plan.
A rejection also names the operator, when Isthmus named one that can be
picked back out. Reading that out of a message is prose-matching, so it
sits next to the check that decides whether a message names a call at
all, and a caller counting what stopped a corpus asks
Rejected.offender() rather than matching on the text itself.
Relations are checked before conversion. Impala's own relations are
unambiguously outside Substrait, and listing them is more use than the
exception Isthmus raises on the first one it meets. This is the
cte_threshold case: above the threshold the repeated subtree becomes an
ImpalaSequence over a producer and one consumer per reference, and the
seam stops being a plan of Calcite relations.
Expressions are left to Isthmus rather than to a list of what it
refuses. Such a list would drift as the Impala operator table changes,
and it would also be wrong: Impala's aggregate classes convert because
Isthmus normalises aggregates by SqlKind, while its scalar functions
resolve on operator identity and do not. Naming an operator
Impala-something says nothing about whether it exports.
What Isthmus cannot resolve by itself is taught to it, and that list is
the other half of this change. Isthmus looks a call up in a map keyed on
the operator, and Calcite compares operators by class, name and kind, so
an Impala operator meaning exactly what Substrait's does still misses.
Additional signatures say which Substrait function each stands for and
leave the operand matching alone.
abs, coalesce, concat, length, floor and ceil with their d-prefixed
spellings, and substr and round;
lag, first_value and last_value for windows, resolved through a
converter of their own that normalises by SqlKind, which passes
Impala's SqlKind.OTHER through to the operator itself;
ndv, which earns its place on the description rather than the name:
Substrait says of approx_count_distinct that it uses HyperLogLog and
Impala's ndv is HyperLogLog too.
Three need more than a name. Impala rewrites IF and cast into operators
carrying SqlKind.OTHER, so the converters Isthmus has for CASE and CAST
never see them, and each gets one of its own. Substrait's substring and
round take an i32 where Impala hands over a bigint and a literal's own
width, so those operands are narrowed first, integers only: Calcite's
EXACT_TYPES also holds DECIMAL, and casting a decimal to an integer
would drop everything after the point without saying so. A datetime plus
arrives as a multiply of the unit by the count, which Substrait has no
form for, so a constant one is folded into the interval literal it works
out to.
What stays refused says why, in the table and in a test, so that a gap
does not read as an oversight. Substrait has no truncate and no
one-argument round. Its approx_count_distinct takes one operand, so an
ndv carrying a precision matches nothing and the precision cannot be
dropped quietly. A datetime plus whose count is not a literal has
nothing to fold. GROUPING has no Substrait function at all, which is
substrait-java#993. And SINGLE_VALUE, which Calcite uses to guard a
scalar subquery and Impala turns into a runtime cardinality check, would
have to become any_value, which picks a row and asserts nothing: that
would turn a query failing on bad data into one quietly returning an
arbitrary row, with nothing in the plan for a consumer to notice.
One mapping is right only under a query option. length becomes
octet_length because Impala counts bytes, measured on a running
instance, but with UTF8_MODE set it counts characters and char_length
is the one that matches. The seam plan carries no options, so nothing
here can tell the two apart; the mapping stays the byte one and says so
where it is written.
An Iceberg snapshot is refused on the same ground. RemoveSnapshotRule
takes the Snapshot node off the plan before the seam, and what is left
is a scan under a name Impala invented for this compilation by
appending _tt_ and a hash of the travel spec. Exported as a named table
that name resolves nowhere, and a consumer falling back to the base name
reads the table as it stands now and reports success. Substrait can say
what was meant -- ReadRel.IcebergTable carries a metadata file and
either a snapshot id or a timestamp, and Impala has all three to hand,
including the timestamp in the microseconds Substrait asks for -- but
substrait-java implements four of the five read types and not that one.
That is substrait-io/substrait-java#1172; until it lands, refusing says
more than a plan that reads the wrong data quietly.
Timestamps leave at the precision Impala stores. ImpalaTypeSystemImpl
reports fifteen fractional digits and Substrait has room for nine, and
Isthmus copies the reported number through, so until now every plan
reading a table with a timestamp column was refused on the way back in
-- whether or not the query named that column, because the read carries
the table's whole schema. An expression's type is narrowed by
overriding TypeConverter.toSubstrait; a schema's is narrowed after the
fact, because toNamedStruct builds its fields with a private
toSubstrait(RelDataType, List<String>) that no override reaches. Types
carry no values, so rewriting one afterwards loses nothing. Three of the
shapes that narrowing covers cannot be reached from a query today and
carry no test: a timestamp with a local timezone, which the planner
refuses to cast before the seam; a literal inside VALUES, which needs
IMPALA-15338; and a timestamp nested in an array or a map, which the
Calcite planner has no types for.
A timestamp literal cannot be narrowed the same way and is refused,
carrying TIMESTAMP_LITERAL. LiteralConverter writes the value in units
of a precision it reads through a static call, so at fifteen digits
2020-01-01 is 1577836800 * 10^15, which wraps to -5454344746497474560
in a signed 64-bit integer; before this the plan exported and carried
that number. Narrowing what the type system reports is a change of its
own.
Caffeine has to be declared for those tests to run at all. Iceberg loads
table metadata through a Caffeine cache, the only unshaded copy reaching
this module rides inside the AWS SDK bundle, and the surefire
configuration drops that jar because it also carries SLF4J 1 and an
older protobuf. Dropping it took Caffeine along, so any Iceberg table
here loaded as an IncompleteTable whose cause was a
NoClassDefFoundError. Nothing noticed because nothing in this module
touched Iceberg before now.
Upstream issues:
https://github.com/substrait-io/substrait-java/issues/993
https://github.com/substrait-io/substrait-java/issues/1163
https://github.com/substrait-io/substrait-java/issues/1172
Testing, in a local cluster on Linux:
- SubstraitPlanExporterTest: 27 tests, 0 failures, 0 errors, covering
each mapping by the Substrait function it produces, each refusal by
the operator or relation named, and the exporter's own contract
- The whole Substrait set in this module, which the two changes above
this one extend: 61 tests, 0 failures
- CalciteOptimizerTest and CalciteSubstraitCompatibilityTest, neither of
which this patch touches: 11 and 4 tests, 0 failures
- Withholding any one of the three converters, or the narrowing, or the
folding, fails the test that covers it, so none of them are vacuous
- Withholding the snapshot check fails its test with "expected ... to be
rejected but it exported", which is the fault itself and not just an
absent guard
- Withholding the timestamp narrowing fails two tests: the one that
reads the precision back out of an exported plan, and the corpus one,
on its first query, with expected:<9> but was:<15>. Withholding the
literal refusal fails its test the same way the snapshot one fails
- Refusing null literals too fails the test that exports one, and
dropping Expression.Literal.PrecisionTimestamp from the walk fails
the test built on a literal by hand, so neither guard is vacuous
- The precision is read back out of the protobuf by walking the message
tree rather than the field a test remembered, so a timestamp arriving
somewhere else is still seen
- A travelled scan is still found through a subquery, a join, a union
and a CTE, checked by hand against all four before settling on the
walk over relation inputs
- git diff --check
Assisted-by: Claude Opus 5 (Anthropic)
Change-Id: I06e7645e6d6af599af6ec3c53780d12935ab7efb
---
M java/calcite-planner/pom.xml
A
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/SubstraitPlanExporter.java
A
java/calcite-planner/src/test/java/org/apache/impala/calcite/service/SubstraitPlanExporterTest.java
A
java/calcite-planner/src/test/java/org/apache/impala/calcite/service/SubstraitProtoTypes.java
5 files changed, 1,670 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/24741/12
--
To view, visit http://gerrit.cloudera.org:8080/24741
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I06e7645e6d6af599af6ec3c53780d12935ab7efb
Gerrit-Change-Number: 24741
Gerrit-PatchSet: 12
Gerrit-Owner: Aleksandr Efimov <[email protected]>
Gerrit-Reviewer: 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: Stamatis Zampetakis <[email protected]>
Gerrit-Reviewer: Steve Carlin <[email protected]>
