andygrove commented on PR #5403:
URL:
https://github.com/apache/datafusion-comet/pull/5403#issuecomment-5441424463
> **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.
This is a good piece of work. Fixing native evaluation rather than routing
through the codegen dispatcher is the right call, and the recursive
`spark_comparator` with nulls-first ordering, dictionary decoding, and struct
lexicographic comparison looks careful. I particularly appreciate that the
description reports the slowdown honestly instead of burying it, and that the
tests compare raw bits rather than numeric equality.
The float slowdown is the thing I want to talk about.
**1.5x to 1.6x on plain float arrays**
`float_extrema` gives up Arrow's vectorized min/max and does a scalar loop
with a per-element null check and two branches. That is a real cost paid by
every user on every `array_min` and `array_max` over a float column, in order
to fix a case that only arises when the array contains both zero signs or
multiple NaN encodings.
There is a fast path available here. Arrow's kernel and Spark disagree only
when the winning value is a zero or a NaN. For any other winner, all tied
candidates are bit-identical, so "keep the first" and "keep whichever Arrow
found" produce the same bytes. So you could run Arrow's kernel first, and only
when the result is `0.0`, `-0.0`, or NaN fall back to the scalar
first-occurrence scan for that one list. Arrays with no zeros and no NaNs,
which is essentially all real data, would then run at DataFusion's speed.
Would that work? If there is a reason it does not, it would be good to say
so in the code, because a permanent 1.6x on the common path to fix an edge case
is a hard trade to justify otherwise.
**`FixedSizeList` input reaches an `exec_err`**
`needs_spark_ordering` returns true for `FixedSizeList` as an element type,
and `spark_comparator` handles it. But `invoke_with_args` matches only
`DataType::List` and `DataType::LargeList` on the outer array and returns
`exec_err!("{} does not support type {other}")` for anything else. So
`array_max` over a `FixedSizeList` column would now fail at runtime rather than
falling back.
I could not find a path where Comet produces a `FixedSizeList` here, so this
may well be unreachable. If it is, could the match arm say so, along the lines
of "Comet only produces List and LargeList; FixedSizeList is unreachable"? And
if it is reachable, this needs either handling or a plan-time `Unsupported`.
**Two smaller things**
`spark_comparator`'s `Dictionary` arm calls `cast(array, value_type)`, which
materializes the whole decoded values array on every invocation. For a heavily
dictionary-encoded column that is a full decode per batch. Is that acceptable,
or worth a note that dictionary elements are the slow path?
The `Cargo.toml` change adds a trailing newline to an unrelated line. Not a
problem, just noting it so it does not look accidental in the diff.
--
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]
