Skip to content

Fix like scalar null#4832

Merged
tustvold merged 2 commits intoapache:masterfrom
tustvold:fix-like-scalar-null
Sep 18, 2023
Merged

Fix like scalar null#4832
tustvold merged 2 commits intoapache:masterfrom
tustvold:fix-like-scalar-null

Conversation

@tustvold
Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #.

Rationale for this change

Found in apache/datafusion#7587

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions Bot added the arrow Changes to the arrow crate label Sep 18, 2023
Copy link
Copy Markdown
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM -- thank you @tustvold

Comment thread arrow-string/src/like.rs
let l_len = l_v.map(|l| l.len()).unwrap_or(l.len());
if r_s {
let scalar = match r_v {
Some(dict) => match dict.nulls().filter(|n| n.null_count() != 0) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the bug the difference in the null check?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug is for the non-dictionary case we didn't check nullability

Comment thread arrow-string/src/like.rs
let b = Scalar::new(StringArray::new_null(1));
let r = like(&a, &b).unwrap();
assert_eq!(r.len(), 1);
assert_eq!(r.null_count(), 1);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest also verifing that r.is_valid(0) == false (though I realize this is technically redundant I think it might capture the intent of the test better)

@tustvold tustvold merged commit 33b881d into apache:master Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arrow Changes to the arrow crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants