andygrove commented on PR #5040:
URL:
https://github.com/apache/datafusion-comet/pull/5040#issuecomment-5441818318
> **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.
There is already `CHANGES_REQUESTED` feedback on this, so I will keep to
points I do not think have been covered.
**No `BinaryType` coverage**
Spark's `Elt` accepts `StringType` or `BinaryType` inputs and returns the
matching type. Both `elt.sql` and `elt_ansi.sql` appear to test only string
arguments. Does `datafusion-spark`'s `SparkElt` handle binary, and does the
serde produce the right return type for it?
This matters more than usual because `CometScalarFunction[Elt]("elt")` does
not set an explicit return type, so the native planner resolves it from the
registry. If binary is unsupported or mistyped, the failure is a native
execution error rather than a fallback. A couple of `elt(i, X'01', X'02')` rows
would settle it either way.
**No performance evidence**
The whole point of moving from the dispatcher to a native UDF is speed, and
the description has no numbers. What does `elt` cost through the dispatcher
versus natively for a realistic batch? Without that, this is a change that
gives up exact-Spark behavior under ANSI for an unquantified benefit. Even a
rough microbenchmark would make the trade explicit.
**Wildcard import**
```scala
-import org.apache.spark.sql.types.{BinaryType, DataTypes, IntegerType,
LongType, StringType}
+import org.apache.spark.sql.types._
```
`strings.scala` uses explicit imports throughout. Swapping to a wildcard
makes it harder to see what the file depends on and can silently shadow names
later. Whatever new type is needed can just be added to the list.
**68 changed lines in `expressions.md`**
That is a lot of churn for one expression's status. If most of it is table
realignment rather than content, could it be reverted so the diff shows only
the `elt` row? It makes the change much easier to review and avoids conflicts
with the other PRs currently touching that file.
--
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]
