You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "discord9 (via GitHub)" <gi...@apache.org> on 2026/07/29 09:02:50 UTC
[PR] fix: preserve dictionary-value nulls in scalar regex operators [datafusion]
discord9 opened a new pull request, #23966:
URL: https://github.com/apache/datafusion/pull/23966
## Which issue does this PR close?
- Closes #23963.
## Rationale for this change
The scalar regex fast path for Dictionary arrays evaluates each dictionary value once and gathers the resulting booleans with `BooleanArray::take_iter`. That gather preserves null keys but drops null validity from the evaluated dictionary values. A key referencing a null dictionary value therefore becomes valid `false`, and negated regex operators can incorrectly turn it into `true`.
This is distinct from #23722, which updates the array-pattern `regex_match_dyn` path. This PR fixes null propagation in `regex_match_dyn_scalar`.
## What changes are included in this PR?
- Replace the validity-dropping dictionary gather with Arrow `take`, preserving both key validity and evaluated child validity.
- Add a physical-expression regression test for `RegexMatch`, `RegexIMatch`, `RegexNotMatch`, and `RegexNotIMatch`. The test covers a null key and a key referencing a null dictionary value, and compares the Dictionary result with the equivalent cast-to-`Utf8` evaluation.
## Are these changes tested?
Yes.
- Focused regression: 1 passed.
- `datafusion-physical-expr`: 1,649 passed, including the new regression.
- Remaining workspace: 7,928 passed, 0 failed, 7 ignored; sqllogictest completed 499/499 files.
- `datafusion` library: 461 passed.
- `datafusion` core integration: 1,057 passed; four local memory-validation runners exceeded their host RSS thresholds and were isolated as environment-sensitive, unrelated to this patch.
- `datafusion-cli`: 106 passed.
- Formatting, workspace clippy with warnings denied, TOML formatting, license headers, changed-file typo checks, and `git diff --check` passed.
## Are there any user-facing changes?
Dictionary-encoded string inputs now preserve SQL null semantics for scalar regex operators. There are no public API changes.
--
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: preserve dictionary-value nulls in scalar regex operators [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #23966:
URL: https://github.com/apache/datafusion/pull/23966#discussion_r3675775369
##########
datafusion/physical-expr/src/expressions/binary/kernels.rs:
##########
@@ -280,12 +280,15 @@ pub(crate) fn regex_match_dyn_scalar(
"Data type {} not supported as a dictionary value type for operation 'regex_match_dyn_scalar' on string array",
other
),
- }.map(
+ }.and_then(
// downcast_dictionary_array duplicates code per possible key type, so we aim to do all prep work before
|evaluated_values| downcast_dictionary_array! {
left => {
- let unpacked_dict = evaluated_values.take_iter(left.keys().iter().map(|opt| opt.map(|v| v as _))).collect::<BooleanArray>();
- Arc::new(unpacked_dict) as ArrayRef
+ Ok(arrow::compute::take(
+ evaluated_values.as_ref(),
Review Comment:
Might be worth adding a one-liner comment on why we use `take`?
##########
datafusion/physical-expr/src/expressions/binary/kernels.rs:
##########
@@ -280,12 +280,15 @@ pub(crate) fn regex_match_dyn_scalar(
"Data type {} not supported as a dictionary value type for operation 'regex_match_dyn_scalar' on string array",
other
),
- }.map(
+ }.and_then(
// downcast_dictionary_array duplicates code per possible key type, so we aim to do all prep work before
|evaluated_values| downcast_dictionary_array! {
left => {
- let unpacked_dict = evaluated_values.take_iter(left.keys().iter().map(|opt| opt.map(|v| v as _))).collect::<BooleanArray>();
- Arc::new(unpacked_dict) as ArrayRef
+ Ok(arrow::compute::take(
+ evaluated_values.as_ref(),
+ left.keys(),
+ None,
+ )?)
},
_ => unreachable!(),
Review Comment:
I don't think we need `downcast_dictionary_array!` anymore.
--
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: preserve dictionary-value nulls in scalar regex operators [datafusion]
Posted by "discord9 (via GitHub)" <gi...@apache.org>.
discord9 commented on code in PR #23966:
URL: https://github.com/apache/datafusion/pull/23966#discussion_r3680214704
##########
datafusion/physical-expr/src/expressions/binary/kernels.rs:
##########
@@ -280,12 +280,15 @@ pub(crate) fn regex_match_dyn_scalar(
"Data type {} not supported as a dictionary value type for operation 'regex_match_dyn_scalar' on string array",
other
),
- }.map(
+ }.and_then(
// downcast_dictionary_array duplicates code per possible key type, so we aim to do all prep work before
|evaluated_values| downcast_dictionary_array! {
left => {
- let unpacked_dict = evaluated_values.take_iter(left.keys().iter().map(|opt| opt.map(|v| v as _))).collect::<BooleanArray>();
- Arc::new(unpacked_dict) as ArrayRef
+ Ok(arrow::compute::take(
+ evaluated_values.as_ref(),
+ left.keys(),
+ None,
+ )?)
},
_ => unreachable!(),
Review Comment:
Good catch — removed it. `AnyDictionaryArray::keys()` gives `take` the type-erased key array directly.
##########
datafusion/physical-expr/src/expressions/binary/kernels.rs:
##########
@@ -280,12 +280,15 @@ pub(crate) fn regex_match_dyn_scalar(
"Data type {} not supported as a dictionary value type for operation 'regex_match_dyn_scalar' on string array",
other
),
- }.map(
+ }.and_then(
// downcast_dictionary_array duplicates code per possible key type, so we aim to do all prep work before
|evaluated_values| downcast_dictionary_array! {
left => {
- let unpacked_dict = evaluated_values.take_iter(left.keys().iter().map(|opt| opt.map(|v| v as _))).collect::<BooleanArray>();
- Arc::new(unpacked_dict) as ArrayRef
+ Ok(arrow::compute::take(
+ evaluated_values.as_ref(),
Review Comment:
Added. `take` is intentional here because it preserves nulls from both the dictionary keys and the evaluated values when expanding back to rows.
--
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: preserve dictionary-value nulls in scalar regex operators [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway merged PR #23966:
URL: https://github.com/apache/datafusion/pull/23966
--
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: preserve dictionary-value nulls in scalar regex operators [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #23966:
URL: https://github.com/apache/datafusion/pull/23966#issuecomment-5117256314
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/23966?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 `82.08955%` with `12 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 80.75%. Comparing base ([`68d5874`](https://app.codecov.io/gh/apache/datafusion/commit/68d587468073c1fc84ffe00db66f35d506dcc828?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`39137d2`](https://app.codecov.io/gh/apache/datafusion/commit/39137d2c4ae3949f19a48b013f6e88624b0c9efe?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/23966?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/physical-expr/src/expressions/binary.rs](https://app.codecov.io/gh/apache/datafusion/pull/23966?src=pr&el=tree&filepath=datafusion%2Fphysical-expr%2Fsrc%2Fexpressions%2Fbinary.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1leHByL3NyYy9leHByZXNzaW9ucy9iaW5hcnkucnM=) | 88.52% | [0 Missing and 7 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/23966?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...on/physical-expr/src/expressions/binary/kernels.rs](https://app.codecov.io/gh/apache/datafusion/pull/23966?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==) | 16.66% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/23966?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 #23966 +/- ##
==========================================
- Coverage 80.75% 80.75% -0.01%
==========================================
Files 1096 1096
Lines 373282 373346 +64
Branches 373282 373346 +64
==========================================
+ Hits 301440 301481 +41
- Misses 53869 53877 +8
- Partials 17973 17988 +15
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/23966?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