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

feat + fix: IPC support for run encoded array. #3662

Merged

tustvold merged 9 commits into apache:master from askoa:run-array-ipc

Feb 10, 2023

Contributor

askoa commented Feb 4, 2023 •

edited

Which issue does this PR close?

Part of #3520

Rationale for this change

See issue description.

What changes are included in this PR?

Change to flat buffer definition file (incorporate the change from arrow project).
Changes to the code generated by flat buffer compiler.
Changes to the script regen.sh that runs flat buffer compiler.
IPC reader, writer and tests for run array.
Ability to unslice the run array before IPC.
Partial implementation of equals method for run array (only supports testing needs).
fix: Handle non-zero offsets in take_run kernel and ArrayAccessor for run array. Iterator for run array is yet to be fixed for non-zero offsets and will be fixed in subsequent PR.

Are there any user-facing changes?

No breaking changes. Users will get new IPC support for run array.

github-actions bot added the arrow label

askoa changed the title ~~<WIP> IPC support for run array.~~ <WIP> IPC support for run encoded array.

askoa changed the title ~~<WIP> IPC support for run encoded array.~~ feat: IPC support for run encoded array.

askoa marked this pull request as ready for review

February 6, 2023 12:26

askoa marked this pull request as draft

February 7, 2023 12:11

Contributor Author

askoa commented Feb 7, 2023 •

edited

I am changing this to draft because I just realized that the IPC writer, written in this PR, for run encoded array does not support offset > 0 or length < original length in ArrayData.

Supporting offset > 0 or length < original length is not a trivial task. The way ArrayData is sliced for RunArray will have implications across all the functions that use ArrayData. For e.g. the recently merged take kernel (#3622) does not work when offset > 0.

My proposal for this PR: The IPC writer will panic if the offset > 0 or length < original length in the input ArrayData. Gradually add support for sliced ArrayData in subsequent PRs. Let me know what you think.

I will mark the PR ready for review after the fix. Will also fix the take kernel for non zero offset in this PR.

ask and others added 2 commits

February 7, 2023 12:17


          Schema.fbs changes, flatbuffer generated code, flatbuffer gen script …

116ec27

…changes

Add ipc reader, writer and equals

Add/Update  tests


          Add support for non zero offset in run array

1f6679f

askoa changed the title ~~feat: IPC support for run encoded array.~~ feat + fix: IPC support for run encoded array.

ask added 3 commits

February 8, 2023 06:01


          clippy fixes

66b8cec


          format fix

1c2bdb4


          doc fix

17ad3a0

askoa marked this pull request as ready for review

February 8, 2023 11:25

askoa mentioned this pull request

fix: Handle sliced array in run array iterator #3681

Merged

tustvold approved these changes

View reviewed changes

Contributor

tustvold left a comment

Really nice work on this, left some comments, I think this is pretty much ready to go as is.

I'll take a look to see if we can't get the archery tests for this enabled

arrow-array/src/array/run_array.rs Outdated Show resolved Hide resolved

arrow-array/src/array/run_array.rs Show resolved Hide resolved

arrow-ipc/src/writer.rs Show resolved Hide resolved

arrow-array/src/array/run_array.rs Outdated Show resolved Hide resolved

arrow-array/src/array/run_array.rs Outdated Show resolved Hide resolved

arrow-array/src/array/run_array.rs Show resolved Hide resolved

arrow-array/src/array/run_array.rs Outdated Show resolved Hide resolved

arrow-data/src/equal/run.rs Outdated Show resolved Hide resolved

arrow-array/src/array/run_array.rs Outdated Show resolved Hide resolved

arrow-array/src/array/run_array.rs Outdated Show resolved Hide resolved


          incorporate pr comments

a7b55e0

viirya reviewed

View reviewed changes

arrow-array/src/array/run_array.rs Outdated Show resolved Hide resolved

devx added 2 commits

February 9, 2023 18:33


          fix formatting

e393355


          more pr comments

64905b0

viirya reviewed

View reviewed changes

arrow-array/src/array/run_array.rs Show resolved Hide resolved

viirya reviewed

View reviewed changes

arrow-data/src/data.rs Outdated Show resolved Hide resolved

viirya reviewed

View reviewed changes

arrow-data/src/equal/run.rs Show resolved Hide resolved

viirya reviewed

View reviewed changes

arrow-ipc/src/writer.rs Outdated Show resolved Hide resolved

viirya approved these changes

View reviewed changes


          pr suggestions

36a2036

tustvold approved these changes

View reviewed changes

Contributor

tustvold left a comment

I think this is good to go, just a minor nit. Will be sure to get this in for the release this afternoon

arrow-array/src/array/run_array.rs Show resolved Hide resolved

arrow-array/src/array/run_array.rs

Comment on lines +233 to +237

+                      let skip_value = if self.offset() > 0 {
+                          self.get_zero_offset_physical_index(self.offset()).unwrap()
+                      } else {
+
+                      };

Contributor

tustvold Feb 10, 2023

Suggested change

      
                    let skip_value = if self.offset() > 0 {
          
                        self.get_zero_offset_physical_index(self.offset()).unwrap()
          
                    } else {
          
                        0
          
                    };
          
                    let skip_value = self.get_physical_index(0).unwrap_or_default();

Perhaps?

Contributor Author

askoa Feb 10, 2023

No. We should get a valid response back. Panic or error is the better option. I used to have error, but based on your earlier feedback changed it to panic.

tustvold merged commit 5b1821e into apache:master

askoa deleted the run-array-ipc branch

February 10, 2023 14:41

ursabot commented Feb 10, 2023

Benchmark runs are scheduled for baseline = 07e2063 and contender = 5b1821e. 5b1821e is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
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

Contributor Author

askoa commented Feb 10, 2023

Benchmark runs are scheduled for baseline = 07e2063 and contender = 5b1821e. 5b1821e is a master commit associated with this PR. Results will be available as each benchmark for each run completes.

@tustvold @viirya Where can I see the benchmark results?

Contributor

tustvold commented Feb 10, 2023

@tustvold @viirya Where can I see the benchmark results?

I don't believe ursabot is setup to benchmark the Rust implementation, I think there was a ticket but I can't seem to find it...

askoa mentioned this pull request

Implement logical comparison for run encoded array #3747

Open

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