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 add_dyn for DictionaryArray support #1951

Merged
merged 3 commits into from Jun 28, 2022
Merged

Add add_dyn for DictionaryArray support #1951

merged 3 commits into from Jun 28, 2022

Conversation

viirya
Copy link
Member

@viirya viirya commented Jun 27, 2022

Which issue does this PR close?

Closes #1950.

Rationale for this change

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 Jun 27, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jun 27, 2022 •
edited

Codecov Report

Merging #1951 (54b52b0) into master (55d6073) will increase coverage by 0.00%.
The diff coverage is 88.46%.

❗ Current head 54b52b0 differs from pull request most recent head c45a338. Consider uploading reports for the commit c45a338 to get more accurate results

@@           Coverage Diff           @@
##           master    #1951   +/-   ##
=======================================
  Coverage   83.47%   83.48%           
=======================================
  Files         221      221           
  Lines       57054    57106   +52     
=======================================
+ Hits        47626    47675   +49     
- Misses       9428     9431    +3
Impacted Files Coverage Δ
arrow/src/compute/kernels/arithmetic.rs 90.61% <88.46%> (-0.26%) ⬇️
parquet/src/encodings/encoding.rs 93.62% <0.00%> (+0.19%) ⬆️
parquet_derive/src/parquet_field.rs 66.21% <0.00%> (+0.22%) ⬆️
arrow/src/datatypes/datatype.rs 65.79% <0.00%> (+0.37%) ⬆️

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 55d6073...c45a338. Read the comment docs.

martin-g
martin-g approved these changes Jun 27, 2022
View reviewed changes
arrow/src/compute/kernels/arithmetic.rs Outdated Show resolved Hide resolved
alamb
alamb approved these changes Jun 28, 2022
View reviewed changes
Copy link
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.

Thank you @viirya -- I went through this PR and it looks good to me.

I am surprised at the amount of code required for this but I tried and couldn't really figure out any better way (as the macros are basically implementing all the type dispatch to then call add).

arrow/src/compute/kernels/arithmetic.rs
.take_iter_unchecked($right.keys_iter())
};

let result = left_iter
Copy link
Contributor

Choose a reason for hiding this comment

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

this is very nice 👌

arrow/src/compute/kernels/arithmetic.rs
fn test_primitive_array_add_dyn_dict() {
let key_builder = PrimitiveBuilder::<Int8Type>::new(3);
let value_builder = PrimitiveBuilder::<Int32Type>::new(2);
let mut builder = PrimitiveDictionaryBuilder::new(key_builder, value_builder);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be good to have at least one null value (via append_null) to make sure that works correctly

@HaoYang670
Copy link
Contributor

I am surprised at the amount of code required for this but I tried and couldn't really figure out any better way (as the macros are basically implementing all the type dispatch to then call add).

I have tried to use nested macro with repetition (the $$) which could reduce some code but make the code less readable. Also this feature has not been stabilized (will be stable in 1.63.0).

HaoYang670
HaoYang670 approved these changes Jun 28, 2022
View reviewed changes
Copy link
Contributor

@HaoYang670 HaoYang670 left a comment

Choose a reason for hiding this comment

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

LGTM!

@HaoYang670
Copy link
Contributor

HaoYang670 commented Jun 28, 2022 •
edited

I am surprised at the amount of code required for this but I tried and couldn't really figure out any better way.

There are lots of places in our code where we map DataType to ArrayPrimitiveType. I am not sure how could we simplify this. Converting value to type is difficult in Rust (I find some discussions of adding dependent type to Rust, such as https://internals.rust-lang.org/t/pre-pre-rfc-dependable-types-in-rust/15500/4, but no much progress).
BTW, const generic is always suggested as a workaround approach, I don't find a good way to use it in our code.
Any idea?

@viirya viirya merged commit 3e06d07 into apache:master Jun 28, 2022
@viirya
Copy link
Member Author

viirya commented Jun 28, 2022

Thanks @alamb @martin-g @HaoYang670

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martin-g martin-g martin-g approved these changes

@alamb alamb alamb approved these changes

@HaoYang670 HaoYang670 HaoYang670 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.

Support DictionaryArray in add kernel
5 participants
@viirya @codecov-commenter @HaoYang670 @martin-g @alamb