andygrove commented on PR #5409:
URL:
https://github.com/apache/datafusion-comet/pull/5409#issuecomment-5441723664
> **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.
The core idea, admit a conservative literal subset to the native kernel by
default rather than gating the whole expression behind `allowIncompatible`, is
a good one, and the reasoning in the `nativeSafeSubset` doc comment about why
kernel compatibility alone is not sufficient (literal round-tripping, eager
child evaluation, offset overflow) is unusually clear.
Four things.
**`BatchSplitExec` does not belong in this PR**
174 lines of new operator plus a planner change that wraps every
`UnnestExec` in a `BatchSplitExec`. That is a runtime change affecting every
`explode` query in Comet, and it has nothing to do with `replace` except that
the offset-overflow reasoning happens to reference it.
It needs its own review: what does the extra stream wrapper cost on the
explode path, does the `SplitMetrics` show up in the SQL UI, and is the
DataFusion 54.1.0 diagnosis right? None of that will get proper attention
buried in a string-expression PR. Could you split it out and make this one
depend on it?
**Columns can also contain malformed UTF-8**
`isNativeSafeStringArg` accepts any `Attribute` or `BoundReference`, and the
doc comment justifies excluding nested expressions on the grounds that "a
throwing or malformed child cannot hide inside the source tree". But a plain
column read from Parquet can absolutely contain bytes that are not valid UTF-8,
which is the whole subject of #4764 and #5310.
What happens when the native `replace` kernel gets a column with invalid
UTF-8? If the answer is "the same thing as every other native string kernel,
and #5310 fixes it at the import boundary", then say so and the concern goes
away. If the answer is that this expression is newly exposing that path by
default where it previously went through the dispatcher, that is worth knowing
before flipping the default.
**The size cap is a heuristic with a sharp edge**
```scala
val maxBytes = Int.MaxValue / math.max(CometConf.COMET_BATCH_SIZE.get(), 1)
```
This assumes the batch reaching the kernel is at most
`spark.comet.batchSize` rows, which the comment acknowledges is only true
because of `BatchSplitExec` on the explode path. Are there other native
operators that can emit an oversized batch? A join, an unnest inside a
different shape, or a scan with a large row group? If any can, the cap is
unsound and the failure mode is an Arrow offset overflow at runtime rather than
a fallback.
Also, `COMET_BATCH_SIZE` is read at plan time. If someone changes it between
planning and execution, or if AQE re-plans, does the cap still hold? A comment
on why plan-time is sufficient would help.
**Test coverage for the newly-default path**
`string_replace.sql` gains 15 lines. Since this flips the default for a
whole class of queries, I would want to see the SQL fixture cover the cases
where a naive `replace` diverges from Spark: overlapping matches
(`replace('aaa', 'aa', 'b')`), a replacement containing the search string,
multibyte search and replacement, a search longer than the source, and a null
source with a non-null search. Are those covered somewhere else already? The
193 lines added to `CometCodegenSuite` look like they are about dispatch
routing rather than result correctness.
--
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]
