fix: support Dictionary arrays in regex_match_dyn (#23709) by Jithendra2608 · Pull Request #23722 · apache/datafusion

Jithendra2608 · 2026-07-20T11:08:34Z

Which issue does this PR close?

This PR fixes the internal error thrown when SIMILAR TO is used with a dictionary-encoded value and a non-scalar array pattern, as described in #23709.
-->

Closes regex_match_dyn (array-array path) does not support Dictionary-encoded needle arrays (SIMILAR TO / ~ operators) #23709 .

What changes are included in this PR?

1.Added a DataType::Dictionary match arm to regex_match_dyn in kernels.rs that mirrors the scalar path.
2.Unpacks the dictionary using the take kernel and routes it back into the standard string matching paths.
3.Added an sqllogictest to regexp_match.slt to verify the fix against the failing query.

Are these changes tested?

adriangb · 2026-07-21T00:35:40Z

 }

-/// Creates a $FUNC(left: ArrayRef, right: ScalarValue) that
-/// downcasts left / right to the appropriate integral type and calls the kernel


Are these comments outdated? CAn we update instead of deleting?

Copilot

Pull request overview

This PR addresses #23709 by extending the regex array-array kernel dispatch (regex_match_dyn) to handle dictionary-encoded string arrays (e.g., Dictionary(Int32, Utf8)) so SIMILAR TO / regex operators don’t error when the value side is dictionary-encoded and the pattern is a non-scalar array.

Changes:

Add a DataType::Dictionary match arm in regex_match_dyn that unpacks dictionary values and reuses the existing string regex paths.
Add an sqllogictest regression case covering the reported failing query shape.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
datafusion/physical-expr/src/expressions/binary/kernels.rs	Adds dictionary handling to `regex_match_dyn` by unpacking dictionary arrays and re-dispatching into existing string regex kernels.
datafusion/sqllogictest/test_files/regexp/regexp_match.slt	Adds a regression test reproducing issue #23709 using `arrow_cast(..., 'Dictionary(Int32, Utf8)') SIMILAR TO ...`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+        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)
+        }


 NULL
+
+
+# Test for Issue 23709: regex_match_dyn with Dictionary encoded patterns


codecov-commenter · 2026-07-21T01:01:18Z

Codecov Report

❌ Patch coverage is 89.47368% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.71%. Comparing base (3a2bc0a) to head (3fd1b57).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...on/physical-expr/src/expressions/binary/kernels.rs	89.47%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #23722      +/-   ##
==========================================
- Coverage   80.71%   80.71%   -0.01%     
==========================================
  Files        1089     1089              
  Lines      368275   368294      +19     
  Branches   368275   368294      +19     
==========================================
- Hits       297260   297256       -4     
- Misses      53301    53318      +17     
- Partials    17714    17720       +6

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

u70b3

A few line-specific notes from a deep dive — overall assessment in a separate comment below.

u70b3 · 2026-07-21T02:30:56Z


 use std::sync::Arc;

-/// Downcasts $LEFT and $RIGHT to $ARRAY_TYPE and then calls $KERNEL($LEFT, $RIGHT)


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?

u70b3 · 2026-07-21T02:30:56Z

        }
+        DataType::Dictionary(_, _) => {
+            let dict = left.as_any_dictionary();
+            let unpacked_left = arrow::compute::kernels::take::take(


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?

u70b3 · 2026-07-21T02:30:56Z

+                dict.keys(),
+                None,
+            )?;
+            regex_match_dyn(&unpacked_left, right, not_match, flag)


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.

u70b3 · 2026-07-21T02:30:56Z

 NULL
+
+
+# Test for Issue 23709: regex_match_dyn with Dictionary encoded patterns


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?

u70b3 · 2026-07-21T02:30:56Z

+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;


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.

u70b3 · 2026-07-21T02:30:56Z

+DROP TABLE dict_regex_t;
+
+statement ok
+DROP TABLE dict_regex_p;


nit: missing trailing newline at end of file.

u70b3 · 2026-07-21T02:33:15Z

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:

Please rebase onto main — among other things it resolves the mixed-width panic I flagged inline on the kernel change.
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:

#[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).

u70b3 · 2026-07-21T06:42:13Z

Confirmed — thanks for the quick turnaround! I pulled the new head (3fd1b57a) and verified locally:

Rebase looks right: the branch now sits on a post-fix: coerce SIMILAR TO operands to a common string type #23704 main, so the mixed-width case (Dictionary(_, Utf8View) / LargeUtf8 value side) is a clean error instead of a panic.
rustfmt --check passes — restoring the doc comments fixed the fmt job.
test_regex_match_dyn_dictionary passes, and the arm is now covered independently of the planner's cast path.
slt wording + trailing newline fixed.

One tiny leftover: the PR body still has stray  markers, so the "Are these changes tested?" section renders inside a comment — worth fixing while waiting on CI.

CI on the new commit hasn't started yet (a maintainer needs to approve the run for first-time contributors). Once it's green, this looks ready to me — nice work!

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

 use arrow::compute::kernels::boolean::not;
 use arrow::compute::kernels::comparison::{regexp_is_match, regexp_is_match_scalar};
+use arrow::compute::kernels::take::take;
 use arrow::datatypes::DataType;


github-actions Bot added physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) labels Jul 20, 2026

Jithendra2608 mentioned this pull request Jul 20, 2026

regex_match_dyn (array-array path) does not support Dictionary-encoded needle arrays (SIMILAR TO / ~ operators) #23709

Open

adriangb reviewed Jul 21, 2026

View reviewed changes

adriangb requested a review from Copilot July 21, 2026 00:36

Copilot started reviewing on behalf of adriangb July 21, 2026 00:36 View session

Copilot AI reviewed Jul 21, 2026

View reviewed changes

github-actions Bot added the auto detected api change Auto detected API change label Jul 21, 2026

u70b3 reviewed Jul 21, 2026

View reviewed changes

JITENDRA added 2 commits July 21, 2026 09:14

fix: support Dictionary arrays in regex_match_dyn (apache#23709)

22d4c94

address reviewer feedback: add unit test, restore docs, fix imports

3fd1b57

Jithendra2608 force-pushed the my-first-fix branch from 2e4d3af to 3fd1b57 Compare July 21, 2026 03:44

adriangb requested a review from Copilot July 21, 2026 06:43

Copilot started reviewing on behalf of adriangb July 21, 2026 06:44 View session

Copilot AI reviewed Jul 21, 2026

View reviewed changes

github-actions Bot removed the auto detected api change Auto detected API change label Jul 21, 2026

JITENDRA and others added 5 commits July 21, 2026 12:50

refactor: use public take API per copilot suggestion

4e96721

Merge branch 'main' into my-first-fix

1244aa0

Merge branch 'main' into my-first-fix

891d1c5

Merge branch 'main' into my-first-fix

ab09e0c

Merge branch 'main' into my-first-fix

ee2a58f

		NULL


		# Test for Issue 23709: regex_match_dyn with Dictionary encoded patterns


		use std::sync::Arc;

		/// Downcasts $LEFT and $RIGHT to $ARRAY_TYPE and then calls $KERNEL($LEFT, $RIGHT)

Uh oh!

Conversation

Jithendra2608 commented Jul 20, 2026

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

Uh oh!

adriangb Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

codecov-commenter commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

u70b3 left a comment

Choose a reason for hiding this comment

Uh oh!

u70b3 Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

u70b3 Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

u70b3 Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

u70b3 Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

u70b3 Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

u70b3 Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

u70b3 commented Jul 21, 2026

Context shift: the #23709 repro is already fixed at the SQL level on main

What I verified about the fix itself

Nits not covered inline

Verdict

Uh oh!

u70b3 commented Jul 21, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Jul 21, 2026 •

edited

Loading