u70b3 commented on code in PR #23722:
URL: https://github.com/apache/datafusion/pull/23722#discussion_r3619023475
##########
datafusion/physical-expr/src/expressions/binary/kernels.rs:
##########
@@ -31,7 +31,7 @@ use datafusion_common::{internal_err, plan_err};
use std::sync::Arc;
-/// Downcasts $LEFT and $RIGHT to $ARRAY_TYPE and then calls $KERNEL($LEFT,
$RIGHT)
Review Comment:
These doc-comment removals (here and on the three macros below) look
unrelated to the fix — and the double blank lines they leave behind are exactly
what fails the `cargo fmt` CI job. Could you restore them?
##########
datafusion/physical-expr/src/expressions/binary/kernels.rs:
##########
@@ -200,6 +196,15 @@ pub(crate) fn regex_match_dyn(
DataType::LargeUtf8 => {
regexp_is_match_flag!(left, right, LargeStringArray, not_match,
flag)
}
+ DataType::Dictionary(_, _) => {
+ let dict = left.as_any_dictionary();
+ let unpacked_left = arrow::compute::kernels::take::take(
+ dict.values().as_ref(),
+ dict.keys(),
+ None,
+ )?;
+ regex_match_dyn(&unpacked_left, right, not_match, flag)
Review Comment:
One case worth knowing about: on this branch's base, a dictionary whose
*value* width differs from the pattern — e.g. `arrow_cast(s, 'Dictionary(Int32,
Utf8View)') SIMILAR TO pat` — unpacks fine here, but this recursive call then
hits `.expect("failed to downcast array")` in `regexp_is_match_flag!` and
**panics** (before this PR, the same query returned a clean "not supported"
internal error, so on this base the PR turns an error into a panic for that
combo). It's the same pre-existing panic class as #22886 for plain arrays, and
#23704 on current main already replaced those `.expect()`s with `exec_err!` —
so rebasing onto main turns this into a clean execution error. I verified that
on a cherry-pick of this commit; more context in my summary comment.
##########
datafusion/physical-expr/src/expressions/binary/kernels.rs:
##########
@@ -200,6 +196,15 @@ pub(crate) fn regex_match_dyn(
DataType::LargeUtf8 => {
regexp_is_match_flag!(left, right, LargeStringArray, not_match,
flag)
}
+ DataType::Dictionary(_, _) => {
+ let dict = left.as_any_dictionary();
+ let unpacked_left = arrow::compute::kernels::take::take(
Review Comment:
nit: this file imports its other arrow kernels at the top (`use
arrow::compute::kernels::...`); mind adding `take` there too instead of using
the fully-qualified path at the call site?
##########
datafusion/sqllogictest/test_files/regexp/regexp_match.slt:
##########
@@ -199,3 +199,22 @@ query B
select null !~* 'abc';
----
NULL
+
+
+# Test for Issue 23709: regex_match_dyn with Dictionary encoded patterns
+statement ok
+CREATE TABLE dict_regex_t AS SELECT * FROM (VALUES ('user auth failed')) v(s);
+
+statement ok
+CREATE TABLE dict_regex_p AS SELECT * FROM (VALUES ('(auth|login)')) v(pat);
+
+query B
+SELECT arrow_cast(dict_regex_t.s, 'Dictionary(Int32, Utf8)') SIMILAR TO
dict_regex_p.pat FROM dict_regex_t CROSS JOIN dict_regex_p;
+----
+true
+
+statement ok
+DROP TABLE dict_regex_t;
+
+statement ok
+DROP TABLE dict_regex_p;
Review Comment:
nit: missing trailing newline at end of file.
##########
datafusion/sqllogictest/test_files/regexp/regexp_match.slt:
##########
@@ -199,3 +199,22 @@ query B
select null !~* 'abc';
----
NULL
+
+
+# Test for Issue 23709: regex_match_dyn with Dictionary encoded patterns
+statement ok
+CREATE TABLE dict_regex_t AS SELECT * FROM (VALUES ('user auth failed')) v(s);
+
+statement ok
+CREATE TABLE dict_regex_p AS SELECT * FROM (VALUES ('(auth|login)')) v(pat);
+
+query B
+SELECT arrow_cast(dict_regex_t.s, 'Dictionary(Int32, Utf8)') SIMILAR TO
dict_regex_p.pat FROM dict_regex_t CROSS JOIN dict_regex_p;
Review Comment:
Heads-up: after a rebase onto current main, this test passes **without
exercising the new kernel arm** — #23704's analyzer coercion now inserts
`CAST(... AS Utf8)` around the dictionary value side (`EXPLAIN` shows
`CAST(CAST(s@0 AS Dictionary(Int32, Utf8)) AS Utf8) ~ pat@1`), so the query
decodes before reaching `regex_match_dyn`. Definitely keep this as end-to-end
regression coverage for the #23709 repro, but please also add a Rust unit test
that calls `regex_match_dyn` with a `DictionaryArray` directly so the arm
itself is covered — an example is in my summary comment.
##########
datafusion/sqllogictest/test_files/regexp/regexp_match.slt:
##########
@@ -199,3 +199,22 @@ query B
select null !~* 'abc';
----
NULL
+
+
+# Test for Issue 23709: regex_match_dyn with Dictionary encoded patterns
Review Comment:
nit: it's the *value* side that is dictionary-encoded here; the pattern is a
plain array. Maybe "Dictionary encoded value side" to avoid confusion?
--
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]
