Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add comparison support for fully qualified BinaryArray #1195

Merged
merged 8 commits into from
Jan 19, 2022
Merged

Add comparison support for fully qualified BinaryArray #1195

merged 8 commits into from
Jan 19, 2022

Conversation

HaoYang670
Copy link
Contributor

Which issue does this PR close?

None. This PR is the first part of #1108.

Rationale for this change

What changes are included in this PR?

We add native comparison support for fully qualified BinaryArray. Support for dynamic BinaryArray will be added in next PR

Are there any user-facing changes?

@github-actions github-actions bot added the arrow Changes to the arrow crate label Jan 18, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jan 18, 2022 •
edited
Loading

Codecov Report

Merging #1195 (ff3ab90) into master (9d637a4) will increase coverage by 0.01%.
The diff coverage is 91.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1195      +/-   ##
==========================================
+ Coverage   82.67%   82.69%   +0.01%     
==========================================
  Files         175      175              
  Lines       51561    51660      +99     
==========================================
+ Hits        42629    42719      +90     
- Misses       8932     8941       +9
Impacted Files Coverage Δ
arrow/src/compute/kernels/comparison.rs 91.96% <91.91%> (-0.01%) ⬇️
arrow/src/datatypes/datatype.rs 66.38% <0.00%> (-0.43%) ⬇️
arrow/src/datatypes/field.rs 53.79% <0.00%> (-0.31%) ⬇️
arrow/src/array/transform/mod.rs 85.43% <0.00%> (-0.14%) ⬇️
parquet_derive/src/parquet_field.rs 65.98% <0.00%> (ø)
arrow/src/array/array_binary.rs 93.48% <0.00%> (+0.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d637a4...ff3ab90. Read the comment docs.

@HaoYang670 HaoYang670 marked this pull request as draft January 18, 2022 03:07
@HaoYang670
Copy link
Contributor Author

Hi teams, I find one of my failures is due to the reference mistake, which I will fix it. But others are caused by unknown feature llvm_asm. What does it mean? I guess I do not modify anything related to it.

@gangliao
Copy link
Contributor

Same error: https://github.com/flock-lab/flock/runs/4849022117?check_suite_focus=true#step:9:696

@gangliao gangliao mentioned this pull request Jan 18, 2022
Merged
2 tasks
@tustvold tustvold mentioned this pull request Jan 18, 2022
Merged
@alamb alamb mentioned this pull request Jan 18, 2022
Closed
@alamb
Copy link
Contributor

alamb commented Jan 18, 2022

Hi @HaoYang670 and @gangliao - sorry for the CI failures. I believe they will be fixed one #1199 is merged

@HaoYang670 HaoYang670 marked this pull request as ready for review January 19, 2022 01:43
@HaoYang670 HaoYang670 closed this Jan 19, 2022
@HaoYang670 HaoYang670 reopened this Jan 19, 2022
tustvold
tustvold reviewed Jan 19, 2022
View reviewed changes
Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

This looks good to me, nice work 👍

I'm not a maintainer so can't approve, but someone should be along soon enough who can get this in

arrow/src/compute/kernels/comparison.rs
assert_eq!(v, expected[i]);
}

let left = LargeBinaryArray::from_vec($left);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice extra coverage 👍

arrow/src/compute/kernels/comparison.rs
}
};
}

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth testing with some non-UTF-8 values, i.e. &[u8], just mix in some stuff in the range b < 128 || b >= 192 which indicate UTF-8 continuation characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your advice @tustvold. I'd like to do a follow-on PR which I will add non-utf8 tests and also add dynamic dispatched comparisons.

arrow/src/compute/kernels/comparison.rs
}

#[test]
fn test_binary_eq_scalar_on_slice() {
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@alamb
Copy link
Contributor

alamb commented Jan 19, 2022

Thank you @HaoYang670 -- this looks great. @tustvold has a good suggestion regarding testing, but we can either do that as part of this PR or as a follow on.

@alamb alamb merged commit 96a9a54 into apache:master Jan 19, 2022
@alamb
Copy link
Contributor

alamb commented Jan 19, 2022

Thanks again @HaoYang670 !

@HaoYang670 HaoYang670 mentioned this pull request Jan 22, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tustvold tustvold tustvold left review comments

@alamb alamb alamb approved these changes

Assignees
No one assigned
Labels
arrow Changes to the arrow crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@HaoYang670 @codecov-commenter @gangliao @alamb @tustvold