u70b3 commented on PR #23722:
URL: https://github.com/apache/datafusion/pull/23722#issuecomment-5029545412
Thanks for the quick turnaround on this, and sorry it took me a while to get
to the review. I went fairly deep: checked out the PR, probed it with extra
sqllogictests on its base, cherry-picked it onto current `main`, and wrote
direct unit tests against `regex_match_dyn`. Line-specific notes are in the
review above; here is the overall picture.
## Context shift: the #23709 repro is already fixed at the SQL level on main
This PR branched off just before #23704 merged (current `main` HEAD). #23704
added `regex_coercion` for `Expr::SimilarTo` in the analyzer, so on current
main the issue's repro no longer reaches `regex_match_dyn` with a Dictionary —
the analyzer inserts a decode cast:
```
EXPLAIN SELECT arrow_cast(t.s, 'Dictionary(Int32, Utf8)') SIMILAR TO p.pat
...
-- ProjectionExec: expr=[CAST(CAST(s@0 AS Dictionary(Int32, Utf8)) AS Utf8)
~ pat@1 ...]
```
I verified the repro returns `true` on main **without** this PR; the same
holds for `~`/`~*`/`!~*` and the regexp UDFs (all coerced via casts). The new
arm remains reachable from physical plans that bypass the analyzer (hand-built
`BinaryExpr`, FFI/Substrait consumers), where it turns an `internal_err!` into
a correct result, and it mirrors the existing `Dictionary` arm in
`regex_match_dyn_scalar` — so I'd frame this as kernel-level robustness rather
than a user-facing fix, and I still think it's worth landing. I'll note the
SQL-level status on #23709.
Two consequences:
1. **Please rebase onto `main`** — among other things it resolves the
mixed-width panic I flagged inline on the kernel change.
2. **After the rebase the sqllogictest no longer covers the arm itself**
(details inline on the test), so please add a direct unit test. This passes on
main + this PR:
```rust
#[test]
fn test_regex_match_dyn_dictionary() -> Result<()> {
let values = StringArray::from(vec![Some("user auth failed"),
Some("anonymous")]);
let keys = Int32Array::from(vec![Some(0), Some(1), None, Some(0)]);
let left: ArrayRef = Arc::new(DictionaryArray::new(keys,
Arc::new(values)));
let right: ArrayRef = Arc::new(StringArray::from(vec![
Some("(auth|login)"), Some("^anon"), Some("x"), Some("nope"),
]));
let result = regex_match_dyn(&left, &right, false, false)?;
assert_eq!(
result.as_any().downcast_ref::<BooleanArray>().unwrap(),
&BooleanArray::from(vec![Some(true), Some(true), None, Some(false)])
);
// negation must preserve nulls
let result = regex_match_dyn(&left, &right, true, false)?;
assert_eq!(
result.as_any().downcast_ref::<BooleanArray>().unwrap(),
&BooleanArray::from(vec![Some(false), Some(false), None, Some(true)])
);
Ok(())
}
```
## What I verified about the fix itself
Probed on the PR's base, where SIMILAR TO applies no coercion and the arm is
genuinely exercised from SQL: multi-row array-array matching with per-row
patterns, `NOT SIMILAR TO` negation preserving NULLs, NULL keys → NULL results,
repeated keys, sliced key arrays (offsets), Int8/UInt8/UInt64 key types, and
the case-insensitive flag — all correct. `Dictionary(_, <non-string>)` values
fail with a clean `internal_err!`, no panic. On the cherry-picked branch, the
unit test above (plus sliced-keys and mixed-width variants) passes as well.
`take`-unpacking is the right approach for the array-array path: patterns
vary per row, so the scalar path's evaluate-on-distinct-values trick can't
apply, and the decode matches what the planner-inserted CAST does for the `~`
operators anyway. The cost is one extra materialization on a path that
previously errored — no benchmark concerns. (Small correction to the PR
description: this doesn't literally "mirror the scalar path" —
`regex_match_dyn_scalar` evaluates on dictionary values and remaps by keys; the
take-unpack here is different, and correctly so.)
## Nits not covered inline
- The PR body has stray `<!--` / `-->` markers, so the "Are these changes
tested?" section renders inside a comment.
## Verdict
Approach is correct and the implementation is sound — concept ACK. Landing
checklist: rebase onto main, add a direct unit test for the arm, and restore
the deleted doc comments (which also fixes the failing `cargo fmt` job).
--
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]
