andygrove commented on PR #5032:
URL:
https://github.com/apache/datafusion-comet/pull/5032#issuecomment-5146853069
@mbutrovich all six threads addressed in 26994c919, details inline. You were
right about the central point, and taking it shrank the PR considerably.
**The whole serde diff is now two mixins.** `CodegenDispatchFallback`
already expresses exactly this routing, and both objects' original
`getSupportLevel` declarations were already the correct classifications, so the
hand-rolled branching is gone:
- `CometStringTranslate` is back to
`CometScalarFunction[StringTranslate]("translate")` plus the mixin —
`"translate"` lives in one place again, and the opted-in `logWarning` comes
back.
- `CometStructsToCsv` is back to the original three-way `getSupportLevel`
plus the mixin. `getUnsupportedReasons` is restored, the duplicated
`nativeSupported` predicate and its two-call-site sync hazard are gone, and the
missing `logDebug` you flagged separately comes for free from the framework's
`Unsupported` arm. I also dropped `CometCodegenDispatch` as the base, since it
is for expressions with no native path at all.
**Tests.** `string_translate.sql` gains the two rows that actually
distinguish the paths, and I verified they discriminate rather than pass
vacuously: adding them to `string_translate_enabled.sql`, which opts into the
native path, fails. `to_csv.sql` gains `sep` and `timestampFormat`/`dateFormat`
coverage. `CometCsvExpressionSuite`'s "to_csv - default options" drops the
`allowIncompatible` wrapper and now covers c6 Double, c10 Timestamp and c11
TimestampNTZ.
**Two things I could not assert, both Spark behavior — verified with
`spark.comet.enabled=false`:**
- **c13 Binary:** Spark's CSV converter renders it via Java's default
`toString()`, e.g. `[B@731af74`. With c13 included, every row mismatches and
the only differing token is that identity hash — the double and both timestamp
columns match exactly. Not assertable by any engine, Spark included.
- **Complex types:** non-null values render as
`org.apache.spark.sql.vectorized.ColumnarArray@1ada50f0` / `ColumnarRow@...` /
`ColumnarMap@...`, and **any null complex value throws `NullPointerException`**
in `UnsafeWriter.write`, with no Comet frames. So `to_csv` over complex types
is either an identity hash or a crash in Spark itself, and there is no
value-parity test to write. I documented this in the fixture instead of pinning
garbage.
Neither is a regression here: those schemas previously fell back to Spark
and now reach the dispatcher, which runs the same `doGenCode`, so the
observable result is identical — the change only decides whether the rest of
the projection stays native. Say the word if you want a separate issue tracking
the complex-type behavior.
**Verified on spark-3.5:** `to_csv.sql`, `string_translate.sql`,
`string_translate_enabled.sql` and both `CometCsvExpressionSuite` tests pass;
spotless and scalastyle clean.
One unrelated note: this branch is behind `main`, which has since added an
**Implementation** column to `expressions.md`. The two Notes cells this PR
changes are still accurate, but the docs table will need regenerating after a
rebase.
--
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]
