andygrove commented on PR #5322: URL: https://github.com/apache/datafusion-comet/pull/5322#issuecomment-5441653949
> **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. Using `Scalar::new` for the broadcast side instead of materializing an N-element array is the right change, and the extracted `get_string_scalar_value` helper removes three copies of the same match. Three things. **"Existing tests" is thin for the path that changed** The scalar-haystack, array-needle branch is described in the code as "less common", and it is the one this PR rewrites. Are there existing tests that cover it? Specifically: - a non-null scalar haystack against an array needle, comparing against the old behavior - a null scalar haystack, which now takes a new early-return path producing `BooleanArray::new_null(needle_array.len())` instead of going through `arrow_contains` - an empty needle array If those are not covered today, this change is unverified. Since the whole point is that this path is rare, it is exactly the one where a regression would sit unnoticed. **Does `arrow_contains` broadcast a scalar on the left?** The previous code always passed two arrays. The new code passes `Scalar::new(...)` as the first argument. Some Arrow comparison kernels only broadcast one side. Presumably this works or the build would fail, but a test result rather than an inference would be reassuring, which loops back to the point above. **Error message consistency** `contains_array_scalar` and `contains_scalar_scalar` both go through `get_string_scalar_value` and produce "contains function requires string type for haystack/needle, got ...". `contains_scalar_array` does not validate at all and lets Arrow produce whatever message it produces for a non-string datum. Since the helper now exists, would you run the haystack scalar through it there too, so all three paths report the same thing? **One process note** The benchmark is an image with no numbers in the text. Images do not survive well in commit history or in a changelog, and they cannot be searched. Could you paste the Criterion medians as a table in the description as well, the way #5174 does? It also makes it much easier for a reviewer to sanity-check the magnitude of the claim. -- 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]
