andygrove commented on PR #4315: URL: https://github.com/apache/datafusion-comet/pull/4315#issuecomment-5441800402
> **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. Lowering `encode(str, 'utf-8')` to a `CAST(string AS binary)` is a neat way to get this for free, and the `CometExprShimCommon` trait shared across the 4.x shims is the right structure for the `StaticInvoke` rewrite. The SQL fixture covers empty strings, NULL, multibyte, and the mixed-case charset literal, which is good. Three things. **The malformed-UTF-8 divergence is not reported to users** Spark replaces malformed bytes during `encode`, while the cast lowering preserves them. That is a wrong answer, not a fallback, and right now the only record of it is an `ignore(...)` line in `encode.sql` plus a code comment. `encode` should report `Incompatible(Some(...))` for this, or at minimum the divergence needs to appear on the string compatibility page so a user can find it. As it stands, `expressions.md` will show `encode` as supported with no caveat and a user with dirty string data gets silently different bytes. That is the exact case Comet's `Incompatible` mechanism exists for. If you would rather not gate it, that is a discussion worth having explicitly, but it should not be an implicit consequence of where the code happens to live. **Charset alias matching** ```scala str.toString.toLowerCase(Locale.ROOT) == "utf-8" ``` Java's `Charset.forName` accepts `UTF8`, `utf8`, and `unicode-1-1-utf-8` as aliases for UTF-8, and Spark accepts whatever `Charset.forName` accepts. So `encode(s, 'UTF8')` falls back here even though it is exactly the case this PR handles. Would `Try(Charset.forName(name).name() == "UTF-8").getOrElse(false)` be better? It handles aliases, and an invalid charset name naturally falls back to Spark, which then raises the proper `INVALID_PARAMETER_VALUE`. **Duplication between the 3.4 and 3.5 shims** The additions to `spark-3.4/CometExprShim.scala` and `spark-3.5/CometExprShim.scala` are identical. The 4.x side already shares through `CometExprShimCommon`. Is there a `spark-3.x` shared location that could host the same dispatch for 3.4 and 3.5? Two copies of a three-line match is minor now, but it is the pattern that makes per-version gaps show up later, and CI only lints a subset of the version matrix. -- 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]
