fix: Fix range out of index error with a temporary workaround by viirya · Pull Request #584 · apache/datafusion-comet

viirya · 2024-06-18T05:45:11Z

Which issue does this PR close?

Closes #540.

Rationale for this change

What changes are included in this PR?

How are these changes tested?

viirya · 2024-06-18T15:47:16Z

I will add a custom Java importer implementation to use the fixed offset field from arrow-rs.

viirya · 2024-06-19T16:07:20Z

Looks like I need to copy more Arrow Java classes to make it work...

viirya · 2024-06-20T07:20:17Z

Looks like I need to copy more Arrow Java classes to make it work...

Instead of a real fix which requires adding offset support to Arrow Java, I added a hacky workaround in my arrow-rs repo, and added custom Java importer implementation.

I re-ran the query and verified this workaround fixes the range out of index error.

viirya · 2024-06-20T07:22:30Z

common/src/main/java/org/apache/arrow/c/CometBufferImportTypeVisitor.java

+      // HACK: For the issue https://github.com/apache/datafusion-comet/issues/540
+      // As Arrow Java doesn't support `offset` in C Data interface, we cannot correctly import
+      // a slice of string from arrow-rs to Java Arrow and then export it to arrow-rs again.
+      // So we add this hack to always take full length of data buffer by assuming the first offset
+      // is always 0 which is true for Arrow Java and arrow-rs.
+      final int len = end;


This is the workaround added in Arrow Java side.

A similar workaround is also added in the custom arrow-rs branch.

This workaround assumes that the first element in offset buffer is always 0. This is true for arrow-rs and Arrow Java although Arrow spec just recommends it but not enforces it. As we only use arrow-rs and Arrow Java, this workaround should work for us without correctness issue.

Are the changes in the custom arrow-rs branch different to the fix that went into arrow-rs master branch recently?

The fix into arrow-rs repo is real fix that correctly reflecting offset of offset buffer into offset field in C Data interface. But only the fix is not enough, we still need the offset support in Arrow Java.

So the workaround here is different but just hacky solution. Instead filing correct offset in C Data interface, we calculate correct data buffer length in a hacky way by assuming the first offset value in offset buffer is always 0 (this is held in both arrow-rs and Arrow Java). So we get correct data buffer length even offset buffer is a slice (i.e., offset-ed in arrow-rs).

viirya · 2024-06-20T20:38:29Z

cc @andygrove This uses a temporary workaround to fix the issue. I'm not sure how long it will take to have the Arrow Java fix, so I think a workaround but not a real fix might be good for us to move forward.

andygrove · 2024-06-21T15:45:59Z

Looks like I need to copy more Arrow Java classes to make it work...

Instead of a real fix which requires adding offset support to Arrow Java, I added a hacky workaround in my arrow-rs repo, and added custom Java importer implementation.

I re-ran the query and verified this workaround fixes the range out of index error.

I also ran the benchmarks and can confirm that this fixes the issue.

viirya · 2024-06-25T22:24:08Z

I proposed the workaround fix to arrow-rs at apache/arrow-rs#5958. If it could be merged in the upstream, we can just use official arrow-rs and DataFusion repos/releases.

viirya · 2024-06-25T22:44:04Z

@andygrove Could you verify again if the latest change still resolves the issue? I changed it to use the commit proposed for arrow-rs apache/arrow-rs#5958 behind a feature flag.

Thanks.

codecov-commenter · 2024-06-25T22:56:08Z

Codecov Report

Attention: Patch coverage is 0% with 192 lines in your changes missing coverage. Please review.

Project coverage is 34.31%. Comparing base (2fc45a2) to head (7c6f7f4).
Report is 19 commits behind head on main.

Files	Patch %	Lines
...g/apache/arrow/c/CometBufferImportTypeVisitor.java	0.00%	132 Missing ⚠️
...in/java/org/apache/arrow/c/CometArrayImporter.java	0.00%	58 Missing ⚠️
...rc/main/java/org/apache/arrow/c/ArrowImporter.java	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #584      +/-   ##
============================================
+ Coverage     34.13%   34.31%   +0.17%     
  Complexity      809      809              
============================================
  Files           106      108       +2     
  Lines         38586    38742     +156     
  Branches       8566     8568       +2     
============================================
+ Hits          13172    13294     +122     
- Misses        22674    22708      +34     
  Partials       2740     2740

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

viirya · 2024-06-26T17:58:33Z

I changed the arrow-rs repo to use the change at apache/arrow-rs#5964. See if everything in CI can pass.

andygrove · 2024-06-27T04:23:57Z

@viirya I confirmed that this fixes the issue when I run the benchmarks locally.

viirya · 2024-06-27T05:32:37Z

@viirya I confirmed that this fixes the issue when I run the benchmarks locally.

Thank you, @andygrove. Then we can merge this and wait for new arrow-rs and DataFusion releases.

andygrove

LGTM. Hopefully we only need to use the forks for 2-3 weeks.

andygrove

I don't think we can depend on tustvold's repo because the branch could get deleted. Could you create a branch in your repo instead?

andygrove · 2024-06-27T17:48:50Z

core/Cargo.toml

-arrow-string = { version = "52.0.0" }
-parquet = { version = "52.0.0", default-features = false, features = ["experimental"] }
+arrow = { git = "https://github.com/tustvold/arrow-rs.git", rev = "78b6139", features = ["prettyprint", "ffi", "chrono-tz"] }
+arrow-array = { git = "https://github.com/tustvold/arrow-rs.git", rev = "78b6139" }


Now that the PR is merged, can we switch this to a revision of the official arrow-rs repo?

Yes. We can point to arrow-rs repo.

viirya · 2024-06-29T06:19:29Z

@andygrove This uses official arrow-rs repo now.

andygrove

Thanks @viirya

viirya · 2024-06-29T17:36:42Z

Thanks @andygrove

…#584) * fix: Fix range out of index error by using custom arrow-rs repo * Add custom Java Arrow classes * Add a hack * Update * Update * Update to use apache/arrow-rs#5958 * Use tustvold's branch * Use official arrow-rs repo

fix: Fix range out of index error by using custom arrow-rs repo

d368098

viirya marked this pull request as draft June 18, 2024 05:45

Add custom Java Arrow classes

8bb4286

Add a hack

17be62a

viirya marked this pull request as ready for review June 20, 2024 07:20

viirya force-pushed the fix_range_index2 branch from 30a80d0 to c7359e6 Compare June 20, 2024 07:22

viirya commented Jun 20, 2024

View reviewed changes

Update

b26a5c2

viirya force-pushed the fix_range_index2 branch from c7359e6 to b26a5c2 Compare June 20, 2024 07:33

Update

ae6d8f5

viirya changed the title ~~fix: Fix range out of index error by using custom arrow-rs repo~~ fix: Fix range out of index error with a temporary workaround Jun 20, 2024

Update to use apache/arrow-rs#5958

b593d96

Use tustvold's branch

9ada354

andygrove approved these changes Jun 27, 2024

View reviewed changes

andygrove requested changes Jun 27, 2024

View reviewed changes

andygrove reviewed Jun 27, 2024

View reviewed changes

Use official arrow-rs repo

7c6f7f4

andygrove approved these changes Jun 29, 2024

View reviewed changes

andygrove merged commit 27288a0 into apache:main Jun 29, 2024

viirya deleted the fix_range_index2 branch June 29, 2024 17:36

Comments

Conversation

viirya commented Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

Uh oh!

viirya commented Jun 18, 2024

Uh oh!

viirya commented Jun 19, 2024

Uh oh!

viirya commented Jun 20, 2024

Uh oh!

viirya Jun 20, 2024

Choose a reason for hiding this comment

Uh oh!

viirya Jun 20, 2024

Choose a reason for hiding this comment

Uh oh!

viirya Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andygrove Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

viirya Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya commented Jun 20, 2024

Uh oh!

andygrove commented Jun 21, 2024

Uh oh!

viirya commented Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

viirya commented Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

viirya commented Jun 26, 2024

Uh oh!

andygrove commented Jun 27, 2024

Uh oh!

viirya commented Jun 27, 2024

Uh oh!

andygrove left a comment

Choose a reason for hiding this comment

Uh oh!

andygrove left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andygrove Jun 27, 2024

Choose a reason for hiding this comment

Uh oh!

viirya Jun 27, 2024

Choose a reason for hiding this comment

Uh oh!

viirya commented Jun 29, 2024

Uh oh!

andygrove left a comment

Choose a reason for hiding this comment

Uh oh!

viirya commented Jun 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

viirya commented Jun 18, 2024 •

edited

Loading

viirya Jun 20, 2024 •

edited

Loading

viirya Jun 21, 2024 •

edited

Loading

viirya commented Jun 25, 2024 •

edited

Loading

viirya commented Jun 25, 2024 •

edited

Loading

codecov-commenter commented Jun 25, 2024 •

edited

Loading

andygrove left a comment •

edited

Loading