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

Use existing array type in take kernel #1046

Merged
merged 2 commits into from Dec 17, 2021
Merged

Use existing array type in take kernel #1046

merged 2 commits into from Dec 17, 2021

Conversation

maxburke
Copy link
Contributor

Closes #1045

@github-actions github-actions bot added the arrow Changes to the arrow crate label Dec 16, 2021
@codecov-commenter
Copy link

codecov-commenter commented Dec 16, 2021 •
edited

Codecov Report

Merging #1046 (01f5c54) into master (fc343e7) will increase coverage by 0.00%.
The diff coverage is 90.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1046   +/-   ##
=======================================
  Coverage   82.30%   82.31%           
=======================================
  Files         168      168           
  Lines       49046    49056   +10     
=======================================
+ Hits        40368    40381   +13     
+ Misses       8678     8675    -3
Impacted Files Coverage Δ
arrow/src/compute/kernels/take.rs 95.14% <90.00%> (-0.08%) ⬇️
parquet_derive/src/parquet_field.rs 65.98% <0.00%> (ø)
parquet/src/encodings/encoding.rs 93.71% <0.00%> (+0.19%) ⬆️
arrow/src/array/transform/mod.rs 85.24% <0.00%> (+0.27%) ⬆️
arrow/src/datatypes/datatype.rs 66.38% <0.00%> (+0.42%) ⬆️

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 fc343e7...01f5c54. Read the comment docs.

alamb
alamb reviewed Dec 16, 2021
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.

makes sense to me @maxburke -- thank you 👍
Any chance you can write a quick test to protect against future regressions?

@maxburke
Copy link
Contributor Author

@alamb done!

alamb
alamb approved these changes Dec 17, 2021
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.

Looks good -- thanks @maxburke

@alamb alamb changed the title Take array type from elements in existing array Use existing array type in take kernel Dec 17, 2021
@alamb alamb merged commit 2d28534 into apache:master Dec 17, 2021
alamb pushed a commit that referenced this pull request Dec 17, 2021
@maxburke @alamb
Use existing array type in take kernel (#1046)
f9489c2 
* Need to use type from data so that we do not lose, for example, timezone information

* add test for take preseving timezone
@alamb alamb added the cherry-picked PR that was backported to active release (will be included in maintenance release) label Dec 17, 2021
@alamb alamb mentioned this pull request Dec 17, 2021
Merged
alamb added a commit that referenced this pull request Dec 20, 2021
@alamb @maxburke
Use existing array type in take kernel (#1046) (#1057)
71ac862 
* Need to use type from data so that we do not lose, for example, timezone information

* add test for take preseving timezone

Co-authored-by: Max Burke <max@urbanlogiq.com>
@tustvold tustvold mentioned this pull request Oct 21, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb approved these changes

Assignees
No one assigned
Labels
arrow Changes to the arrow crate cherry-picked PR that was backported to active release (will be included in maintenance release)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Arrays generated by take lack timezone information
3 participants
@maxburke @codecov-commenter @alamb