andygrove commented on PR #5472:
URL:
https://github.com/apache/datafusion-comet/pull/5472#issuecomment-5441247625
> **Note on this review:** this was generated by an LLM (Claude Code) at my
request while I worked through a review backlog. I have not verified the
individual findings myself. Please treat everything below as suggestions to
evaluate rather than as authoritative review feedback, and push back on
anything that is wrong or already handled.
Thanks for tracking this down. The root cause is clear and the fix lines up
with how Spark's own `NormalizeFloatingNumbers` decides what needs normalizing,
so the direction looks right to me. The test that builds the payload NaNs
programmatically and compares boolean outputs rather than relying on the
NaN-aware answer checker is a nice touch.
A few things I would like to understand better before this goes in.
**Constant-folding the literal instead of wrapping it**
In `CometExecRule.scala` the fall-through now wraps the literal in
`KnownFloatingPointNormalized(NormalizeNaNAndZero(expr))`. For a literal we
already know the value at plan time, so we could just replace it with the
canonical literal (`Literal(Float.NaN, FloatType)` or `Literal(0.0f,
FloatType)`) and skip emitting a normalization node into the serialized plan
entirely. That keeps the plan smaller and avoids asking the native side to
evaluate a normalization over a constant. Was there a reason to prefer the
wrapper here, or is folding worth doing?
**`In` and `InSet` are not in the normalize list**
`normalize` rewrites `EqualTo`, `EqualNullSafe`, the four ordering
comparisons, `Divide`, and `Remainder`, but not `In` or `InSet`. Both are
registered in `QueryPlanSerde` (`CometIn`, `CometInSet`). Spark's `In.eval`
compares through the type's `Ordering`, which puts all NaNs in one equivalence
class, so `WHERE value IN (double('NaN'))` matches NaN rows in Spark. Does the
native path give the same answer today, or does this have the same class of bug
the PR is fixing for `EqualTo`? If it does, I would rather see it either
covered here or filed as a tracking issue and linked from this PR, since it is
the same user-visible symptom of rows silently disappearing from a filter.
**Scope of `normalizePlan`**
`normalizePlan` only transforms `ProjectExec` and `FilterExec`. That is
presumably deliberate because Spark's own rule covers join keys and window
partition specs. It would help to have a short comment there saying which
operators intentionally rely on Spark's rule, so the next person does not have
to reconstruct that reasoning. #5469 seems to be filling in the sort and window
rank side of this. Is there an umbrella issue tying these together?
**Test coverage question**
The test drives the comparison operators through `CometProjectExec` and only
the `===` case through `CometFilterExec`. Would it be worth pushing the
ordering comparisons through the filter path too? The filter path is where a
wrong answer silently drops rows rather than producing a visibly wrong boolean
column, so that is the case users are most likely to hit.
--
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]
