Skip to content

GH-32105: [C++] Encode and decode Run-End Encoded vectors - #34195

Merged
zeroshade merged 20 commits into
apache:mainfrom
felipecrv:ree_enc_dec
Mar 13, 2023
Merged

GH-32105: [C++] Encode and decode Run-End Encoded vectors#34195
zeroshade merged 20 commits into
apache:mainfrom
felipecrv:ree_enc_dec

Conversation

@felipecrv

@felipecrv felipecrv commented Feb 15, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

@github-actions

github-actions Bot commented Feb 15, 2023

Copy link
Copy Markdown

@github-actions

github-actions Bot commented Feb 15, 2023

Copy link
Copy Markdown

⚠️ GitHub issue #32105 has been automatically assigned in GitHub to PR creator.

@github-actions

github-actions Bot commented Feb 25, 2023

Copy link
Copy Markdown

⚠️ GitHub issue #32105 has been automatically assigned in GitHub to PR creator.

@felipecrv
felipecrv force-pushed the ree_enc_dec branch 3 times, most recently from 47b133f to 5ef4498 Compare February 27, 2023 19:19
@felipecrv
felipecrv marked this pull request as ready for review February 27, 2023 19:22
@felipecrv

felipecrv commented Feb 27, 2023

Copy link
Copy Markdown
Contributor Author

@westonpace @zeroshade @lidavidm this is now ready for review.

@felipecrv

felipecrv commented Feb 27, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I just noticed the input type matcher isn't really necessary for these kernels, so I will remove that commit next time I force push with more changes. That saves one round of CI builds.

@github-actions

github-actions Bot commented Feb 27, 2023

Copy link
Copy Markdown

⚠️ GitHub issue #32105 has been automatically assigned in GitHub to PR creator.

@felipecrv felipecrv mentioned this pull request Feb 28, 2023
Closed
6 tasks done
lidavidm
lidavidm reviewed Feb 28, 2023
View reviewed changes

@lidavidm lidavidm left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I need to take a closer look at the kernel implementations themselves, but here are some initial comments

Comment thread cpp/src/arrow/compute/api_vector.h Resolved
Comment thread cpp/src/arrow/compute/api_vector.h Resolved
Comment thread cpp/src/arrow/compute/kernels/vector_run_end_encode_test.cc Resolved
Comment thread cpp/src/arrow/compute/kernels/vector_run_end_encode_test.cc Resolved
Comment thread cpp/src/arrow/compute/kernels/vector_run_end_encode_test.cc Resolved
Comment thread cpp/src/arrow/compute/kernels/vector_run_end_encode.cc Resolved
Comment thread cpp/src/arrow/compute/kernels/vector_run_end_encode.cc Resolved
@felipecrv
felipecrv force-pushed the ree_enc_dec branch 2 times, most recently from 5c296a6 to c8c2661 Compare March 7, 2023 00:37
@github-actions

github-actions Bot commented Mar 7, 2023

Copy link
Copy Markdown

⚠️ GitHub issue #32105 has been automatically assigned in GitHub to PR creator.

@felipecrv

felipecrv commented Mar 7, 2023

Copy link
Copy Markdown
Contributor Author

@westonpace @zeroshade this is now ready for final review. I've addressed all of @lidavidm's comments.

zeroshade
zeroshade requested changes Mar 8, 2023
View reviewed changes

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just had a few nitpicks really, overall looks really good to me.

Comment thread cpp/src/arrow/compute/kernels/vector_run_end_encode.cc Resolved
Comment thread cpp/src/arrow/compute/kernels/vector_run_end_encode.cc Resolved
Comment thread cpp/src/arrow/compute/kernels/vector_run_end_encode.cc Resolved
Comment thread cpp/src/arrow/compute/kernels/vector_run_end_encode.cc Resolved
Comment thread cpp/src/arrow/compute/kernels/vector_run_end_encode.cc Resolved
Comment thread cpp/src/arrow/compute/kernels/vector_run_end_encode.cc Resolved
Comment thread cpp/src/arrow/compute/kernels/vector_run_end_encode.cc Resolved
Comment thread cpp/src/arrow/compute/kernels/vector_run_end_encode.cc Resolved
Comment thread cpp/src/arrow/compute/kernels/vector_run_end_encode.cc Resolved
@felipecrv
felipecrv requested review from zeroshade and removed request for westonpace March 9, 2023 01:10
@felipecrv
felipecrv requested a review from zeroshade March 13, 2023 14:57
zeroshade
zeroshade approved these changes Mar 13, 2023
View reviewed changes

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM the code definitely looks a bit simpler with the extracted prealloc method + early return.... if i do say so myself 😛

@zeroshade
zeroshade merged commit dbc1e90 into apache:main Mar 13, 2023
@felipecrv
felipecrv deleted the ree_enc_dec branch March 13, 2023 17:35
@ursabot

ursabot commented Mar 13, 2023

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = afa362e and contender = dbc1e90. dbc1e90 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.18% ⬆️0.3%] test-mac-arm
[Finished ⬇️0.77% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.69% ⬆️0.16%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] dbc1e90b ec2-t3-xlarge-us-east-2
[Finished] dbc1e90b test-mac-arm
[Finished] dbc1e90b ursa-i9-9960x
[Finished] dbc1e90b ursa-thinkcentre-m75q
[Finished] afa362e5 ec2-t3-xlarge-us-east-2
[Failed] afa362e5 test-mac-arm
[Finished] afa362e5 ursa-i9-9960x
[Finished] afa362e5 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

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

Reviewers

@lidavidm lidavidm lidavidm left review comments
@zeroshade zeroshade zeroshade approved these changes

Assignees

No one assigned

Labels

Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[C++] Implement encode and decode functions for Run-Length encoding

5 participants

@felipecrv @ursabot @lidavidm @zeroshade @zagto