andygrove commented on PR #4802:
URL:
https://github.com/apache/datafusion-comet/pull/4802#issuecomment-5607100818
Thanks @comphead. All four are settled, and I did the five minor notes in
the same commit rather than a follow-up since they were all small.
**Docs.** You were right that the page contradicted the PR. `hll_*` is out
of the not-planned bullet, which now names only `kll_sketch_*`, `theta_*`,
`count_min_sketch`, `bitmap_*` and `approx_top_k*`, and the four functions have
rows in `agg_funcs` and `misc_funcs`. I also added a short paragraph next to
the existing `approx_count_distinct` note explaining why this is the one sketch
family we accelerate — the sketches are mutually readable with Spark's, and
only the merged estimate drifts — so the reasoning lives on the page rather
than only in the PR description. I did not hand-fill the Implementation column:
I ran `generate-docs` against `spark-4.0` and `spark-4.1` into a scratch tree
and diffed, and all four cells come back `Native` exactly as committed. The
generated compatibility pages now carry the error-class divergence and the
HLL_4 rejection per expression too.
Tracking issue is #5814, and `Closes #.` now points at it.
**The minor notes.** `update_i32`/`update_i16`/`update_i8` are gone; the
accumulator was already widening with `as i64`, which sign-extends the same
way, so they had never been called. `update_batch` downcasts the array once now
instead of building a `ScalarValue` per row, which was copying every string and
binary value onto the heap just to hash it and drop it. I did not want to take
the equivalence on trust, so
`every_input_type_hashes_the_same_as_a_direct_update` asserts byte equality
against a sketch fed the same values directly, per input type, including
negatives for the sign extension and the empty string that both paths skip.
`from_bytes` returns `Execution` rather than `Internal` — invalid sketch bytes
are user data, and it is also what the "surfaces as a plain Comet execution
error" note already promised. `getUnsupportedReasons()` was missing the
lgConfigK range case on `CometHllSketchAgg` and was absent entirely on
`CometHllUnion` and `CometHllUnionAgg`, both of which
return `Unsupported` for a non-foldable flag; all three are complete. And
`datasketches` is pinned to `=0.3.0` with a comment saying why, since both the
compact workaround and the aux guard are written against that release's array
code.
While I was in the incompatibility lists I recorded the HLL_4 rejection
itself as one, on the three serdes that read sketch bytes. Comet errors there
where Spark returns an estimate, and by your own argument that belongs in front
of anyone opting in rather than being discovered at runtime.
**One correction on the `hll_union` NULL flag.** The fix is right and I have
kept it, but I do not think the query reproduces under the default optimizer,
and the severity is worth being accurate about. Spark 4.x marks `HllUnion`
null-intolerant:
```scala
// datasketchesExpressions.scala
case class HllUnion(first: Expression, second: Expression, third: Expression)
extends TernaryExpression with CodegenFallback with ExpectsInputTypes {
override def nullIntolerant: Boolean = true
```
and `NullPropagation` has a generic rule for exactly that:
```scala
// optimizer/expressions.scala
// Non-leaf NullIntolerant expressions will return null, if at least one of
its children is
// a null literal.
case e if e.nullIntolerant && e.children.exists(isNullLiteral) =>
Literal.create(null, e.dataType)
```
`cast(null as boolean)` is constant-folded to `Literal(null, BooleanType)`
in the same batch, so the whole `HllUnion` becomes a NULL literal in the
optimized plan and never reaches the serde. And since the serde requires
`third.foldable`, any NULL flag that passes that gate is also one
`NullPropagation` removes — so the path is reachable only when a user excludes
that rule. Still a genuinely wrong answer, just not one you hit by default.
That matters for the test more than for the fix. Written the obvious way it
would have passed on a folded plan without ever executing the kernel, which is
the same trap as the `checkSparkAnswerMaybeThrows` one you caught. So
`hll_union with a NULL allowDifferentLgConfigK returns NULL` excludes
`NullPropagation`, asserts `hll_union` actually survives into the plan, asserts
the result is NULL, and then compares against Spark with the same rule excluded.
`CometHllUnionAgg` I left alone for the reason you gave — `convert` falls
back when `right.eval()` is not a `Boolean`, and Spark's
`null.asInstanceOf[Boolean]` gives `false`, so the two agree whichever way the
flag arrives. That asymmetry is written down next to the null check now so it
does not read as an oversight later.
--
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]
