Re: [PR] chore: Use native impl of elt function [datafusion-comet]

["andygrove (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org