sam-1112 commented on PR #5409:
URL:
https://github.com/apache/datafusion-comet/pull/5409#issuecomment-5469029868
Thanks Andy and Chao for the review and clarifications.
I agree that this PR has grown beyond the original `replace` routing change.
The malformed-column path and the global runtime batch bound are not
established well enough to call the current default-native subset safe.
I’ll do the following next:
1. Split `BatchSplitExec`, the `UnnestExec` integration, metric forwarding
and SQL UI behavior, and wrapper-overhead evaluation into a separate PR. That
runtime change needs an independent review regardless of what remains in this
PR.
2. I will not assume that #5310 is already protecting column inputs. Until
an import-boundary fix is merged, column source and replacement arguments will
remain on the dispatcher. Default-native `replace` for column inputs can be
reconsidered in a separate PR after that protection has landed.
3. I will remove the `Int.MaxValue / spark.comet.batchSize` eligibility
heuristic. Without an enforced runtime batch bound, broadcasting a literal into
a `Utf8` array cannot be considered safe. Combined with keeping column
arguments on the dispatcher, this means that I will not enable native `replace`
by default in this PR. I am not planning to make the kernel scalar-aware here.
4. I’ll add explicit SQL result-parity cases for:
- a multibyte replacement; and
- a replacement containing the search string.
The earlier regressions involving malformed literals, NULL short-circuiting,
nested source expressions, the post-`explode` reproducer, and forwarded
`UnnestExec` metrics pass on the current head, and the current CI is green.
I’ll leave the source-safety and runtime-batch threads open until
`BatchSplitExec` has been split out and the default-native change has been
removed from this PR.
After splitting out `BatchSplitExec`, I’ll keep this PR only for the
`replace` test coverage and for dropping the unsafe default-native subset. If
nothing remains after that cleanup, I’ll close it and revisit default-native
`replace` after the import-boundary and runtime batch-bound work has landed.
--
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]
