andygrove commented on issue #4764:
URL:
https://github.com/apache/datafusion-comet/issues/4764#issuecomment-5192107963
## Note for Gap B: import site C now has consumers that depend on it *not*
decoding
Two things about import site C (JVM UDF result, `from_ffi` at
`native/spark-expr/src/jvm_udf/mod.rs:239`) have changed since the Gap B
investigation notes above were written. Both affect how Gap B should be
designed, so recording them here.
### Site C is no longer just user UDFs
The table above describes site C as "Result of a JVM-side Spark UDF",
hotness "per UDF eval". That is now an undercount. Since #4728 routes
`Unsupported` (not just `Incompatible`) through the JVM codegen dispatcher,
every serde mixing in `CodegenDispatchFallback` returns its results through
site C. There are currently 21 such serdes, covering ordinary built-in
expressions on default config, not just registered user UDFs.
So site C is closer in character to site A than the table suggests: it
carries built-in expression results on default-config queries. That matters for
the benchmarking task, and it matters for the blast radius of any decode
inserted there.
### A decode at site C would reverse a fix that is currently in review
PR #5267 fixes `spark.sql.function.concatBinaryAsString=true` producing
wrong bytes, by marking the affected `concat` `Unsupported` so Spark's
generated code runs it inside the Comet pipeline and the original bytes
survive. The mechanism only works because site C passes bytes through
unchecked. The JVM writes the raw `UTF8String` bytes into the `VarCharVector`
(`CometBatchKernelCodegenOutput.scala:243`), and native `hex` reads them back
out.
If Gap B decodes imported `Utf8`/`LargeUtf8` at site C unconditionally,
`hex(concat(cast(b AS string), x))` goes back to returning `EFBFBDEFBFBD41` and
that PR's SQL file test starts failing. The revert would be silent at review
time and only show up as a red test.
Whether that is a problem depends on which way we go on the underlying
policy question, which is really the first task in this EPIC ("decide the
ingress policy"). #5267 is effectively arguing for an option not listed under
"Alternatives considered": keep byte-exact subtrees out of native execution
entirely, so that the bytes never need to be representable in Arrow `Utf8`.
That option needs site C to be exempt from Gap B's decode, plus a rule for
which expressions qualify. The alternative is that we hold the line from #4763
(decode, accept the documented byte-round-trip divergence) and #5267 becomes a
docs change instead.
Either way, Gap B should not treat site C as a mechanical "add a decode
here" the way sites A and B are. Flagging so whoever picks it up makes that
call deliberately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
