You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Jithendra2608 (via GitHub)" <gi...@apache.org> on 2026/07/20 11:08:38 UTC

[PR] fix: support Dictionary arrays in regex_match_dyn (#23709) [datafusion]

Jithendra2608 opened a new pull request, #23722:
URL: https://github.com/apache/datafusion/pull/23722

   ## 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 #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?
   
   <!--
   Yes.
   Added a new sqllogictest to datafusion/sqllogictest/test_files/regexp/regexp_match.slt. This test specifically covers the reproducer from #23709, verifying that regex_match_dyn successfully evaluates a Dictionary(Int32, Utf8) encoded array against a non-scalar array pattern without throwing an internal error.
   -->
   
   
   


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org


Re: [PR] fix: support Dictionary arrays in regex_match_dyn (#23709) [datafusion]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #23722:
URL: https://github.com/apache/datafusion/pull/23722#issuecomment-5028889861

   <!-- semver-check-comment -->
   Thank you for opening this pull request!
   
   Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks) reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).
   
   <details>
   <summary>Details</summary>
   
   ```
        Cloning apache/main
       Building datafusion-physical-expr v54.0.0 (current)
          Built [  35.552s] (current)
        Parsing datafusion-physical-expr v54.0.0 (current)
         Parsed [   0.054s] (current)
       Building datafusion-physical-expr v54.0.0 (baseline)
          Built [  30.145s] (baseline)
        Parsing datafusion-physical-expr v54.0.0 (baseline)
         Parsed [   0.054s] (baseline)
       Checking datafusion-physical-expr v54.0.0 -> v54.0.0 (no change; assume patch)
        Checked [   0.539s] 223 checks: 221 pass, 2 fail, 0 warn, 30 skip
   
   --- failure function_parameter_count_changed: pub fn parameter count changed ---
   
   Description:
   A publicly-visible function now takes a different number of parameters.
           ref: https://doc.rust-lang.org/cargo/reference/semver.html#fn-change-arity
          impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/function_parameter_count_changed.ron
   
   Failed in:
     datafusion_physical_expr::create_physical_sort_exprs now takes 3 parameters instead of 4, in /home/runner/work/datafusion/datafusion/datafusion/physical-expr/src/physical_expr.rs:211
     datafusion_physical_expr::planner::create_physical_expr now takes 3 parameters instead of 4, in /home/runner/work/datafusion/datafusion/datafusion/physical-expr/src/planner.rs:114
     datafusion_physical_expr::create_physical_expr now takes 3 parameters instead of 4, in /home/runner/work/datafusion/datafusion/datafusion/physical-expr/src/planner.rs:114
     datafusion_physical_expr::create_physical_partitioning now takes 3 parameters instead of 4, in /home/runner/work/datafusion/datafusion/datafusion/physical-expr/src/physical_expr.rs:223
     datafusion_physical_expr::planner::create_physical_exprs now takes 3 parameters instead of 4, in /home/runner/work/datafusion/datafusion/datafusion/physical-expr/src/planner.rs:575
     datafusion_physical_expr::create_physical_exprs now takes 3 parameters instead of 4, in /home/runner/work/datafusion/datafusion/datafusion/physical-expr/src/planner.rs:575
     datafusion_physical_expr::create_physical_sort_expr now takes 3 parameters instead of 4, in /home/runner/work/datafusion/datafusion/datafusion/physical-expr/src/physical_expr.rs:199
   
   --- failure method_parameter_count_changed: pub method parameter count changed ---
   
   Description:
   A publicly-visible method now takes a different number of parameters, not counting the receiver (self) parameter.
           ref: https://doc.rust-lang.org/cargo/reference/semver.html#fn-change-arity
          impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/method_parameter_count_changed.ron
   
   Failed in:
     datafusion_physical_expr::aggregate::LoweredAggregateBuilder::new takes 5 parameters in /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/8a43071b67a4717025d38da73af2e81c0622de98/datafusion/physical-expr/src/aggregate.rs:440, but now takes 4 parameters in /home/runner/work/datafusion/datafusion/datafusion/physical-expr/src/aggregate.rs:434
   
        Summary semver requires new major version: 2 major and 0 minor checks failed
       Finished [  67.877s] datafusion-physical-expr
       Building datafusion-sqllogictest v54.0.0 (current)
          Built [ 200.385s] (current)
        Parsing datafusion-sqllogictest v54.0.0 (current)
         Parsed [   0.025s] (current)
       Building datafusion-sqllogictest v54.0.0 (baseline)
          Built [ 205.902s] (baseline)
        Parsing datafusion-sqllogictest v54.0.0 (baseline)
         Parsed [   0.024s] (baseline)
       Checking datafusion-sqllogictest v54.0.0 -> v54.0.0 (no change; assume patch)
        Checked [   0.117s] 223 checks: 223 pass, 30 skip
        Summary no semver update required
       Finished [ 409.693s] datafusion-sqllogictest
   ```
   
   </details>


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org


Re: [PR] fix: support Dictionary arrays in regex_match_dyn (#23709) [datafusion]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #23722:
URL: https://github.com/apache/datafusion/pull/23722#discussion_r3620007885


##########
datafusion/physical-expr/src/expressions/binary/kernels.rs:
##########
@@ -25,6 +25,7 @@ use arrow::compute::kernels::bitwise::{
 };
 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;

Review Comment:
   Prefer importing the public `arrow::compute::take` API rather than `arrow::compute::kernels::take::take`. Other DataFusion code uses `arrow::compute::take` (e.g. dictionary handling in `physical-expr/src/expressions/in_list/static_filter.rs`), and depending on the `kernels::*` module path makes this code more brittle to Arrow internal refactors.



-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org


Re: [PR] fix: support Dictionary arrays in regex_match_dyn (#23709) [datafusion]

Posted by "u70b3 (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org


Re: [PR] fix: support Dictionary arrays in regex_match_dyn (#23709) [datafusion]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #23722: fix: support Dictionary arrays in regex_match_dyn (#23709)
URL: https://github.com/apache/datafusion/pull/23722


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org


Re: [PR] fix: support Dictionary arrays in regex_match_dyn (#23709) [datafusion]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #23722:
URL: https://github.com/apache/datafusion/pull/23722#issuecomment-5754364785

   Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org


Re: [PR] fix: support Dictionary arrays in regex_match_dyn (#23709) [datafusion]

Posted by "u70b3 (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org


Re: [PR] fix: support Dictionary arrays in regex_match_dyn (#23709) [datafusion]

Posted by "u70b3 (via GitHub)" <gi...@apache.org>.
u70b3 commented on PR #23722:
URL: https://github.com/apache/datafusion/pull/23722#issuecomment-5030932619

   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-#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!
   


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org


Re: [PR] fix: support Dictionary arrays in regex_match_dyn (#23709) [datafusion]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #23722:
URL: https://github.com/apache/datafusion/pull/23722#discussion_r3618579463


##########
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:
   The test comment says "Dictionary encoded patterns", but the reproducer (and the query below) dictionary-encodes the **value** side (`dict_regex_t.s`), not the pattern. This is misleading when someone later looks for dictionary pattern coverage.



##########
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:
   `regex_match_dyn` now unwraps Dictionary-encoded `left` via `take` and then recursively dispatches using the *unpacked* left type. If `right` is still Dictionary-encoded (or otherwise a different string representation such as `Utf8View` vs `Utf8`), the downstream `regexp_is_match_flag!` macro will attempt to downcast `right` to the same array type as `left` and will panic on the `expect("failed to downcast array")` path. This is a behavior regression from returning an internal error for Dictionary types, and can surface if both operands are dictionary-encoded (or have been coerced to different string view types).



-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org


Re: [PR] fix: support Dictionary arrays in regex_match_dyn (#23709) [datafusion]

Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #23722:
URL: https://github.com/apache/datafusion/pull/23722#discussion_r3618548412


##########
datafusion/physical-expr/src/expressions/binary/kernels.rs:
##########
@@ -103,8 +101,6 @@ macro_rules! call_scalar_kernel {
     }};
 }
 
-/// Creates a $FUNC(left: ArrayRef, right: ScalarValue) that
-/// downcasts left / right to the appropriate integral type and calls the kernel

Review Comment:
   Are these comments outdated? CAn we update instead of deleting?



-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org


Re: [PR] fix: support Dictionary arrays in regex_match_dyn (#23709) [datafusion]

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #23722:
URL: https://github.com/apache/datafusion/pull/23722#issuecomment-5029000171

   ## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/23722?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :x: Patch coverage is `85.71429%` with `1 line` in your changes missing coverage. Please review.
   :white_check_mark: Project coverage is 80.69%. Comparing base ([`3b1ce16`](https://app.codecov.io/gh/apache/datafusion/commit/3b1ce16be6beb3377c59ec36fd9f8ae62998c4c0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`2e4d3af`](https://app.codecov.io/gh/apache/datafusion/commit/2e4d3af14fc42d57a0874e3f10ea6cb84007efd6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   :warning: Report is 5 commits behind head on main.
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/23722?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [...on/physical-expr/src/expressions/binary/kernels.rs](https://app.codecov.io/gh/apache/datafusion/pull/23722?src=pr&el=tree&filepath=datafusion%2Fphysical-expr%2Fsrc%2Fexpressions%2Fbinary%2Fkernels.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1leHByL3NyYy9leHByZXNzaW9ucy9iaW5hcnkva2VybmVscy5ycw==) | 85.71% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/23722?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@            Coverage Diff             @@
   ##             main   #23722      +/-   ##
   ==========================================
   - Coverage   80.69%   80.69%   -0.01%     
   ==========================================
     Files        1088     1088              
     Lines      367766   367773       +7     
     Branches   367766   367773       +7     
   ==========================================
   - Hits       296781   296776       -5     
   - Misses      53276    53284       +8     
   - Partials    17709    17713       +4     
   ```
   </details>
   
   [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/23722?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org