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.

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

ffi_stream.rs: Align buffers when importing arrays #4

Merged
merged 2 commits into from
Feb 17, 2025

Conversation

felipecrv
Copy link

@felipecrv felipecrv commented Feb 15, 2025

Upstream PR: apache#7137

Discussion in arrow-adbc apache/arrow-adbc#2526

@github-actions github-actions bot added the arrow label Feb 15, 2025
@felipecrv felipecrv requested a review from gliga February 15, 2025 22:16
@gliga
Copy link

gliga commented Feb 15, 2025

Maybe link to the PR you opened to the forked repo and also to an example that led us to discover the issue, e.g., https://github.com/EngineeringSoftware/use-arrow-adbc/blob/89d83a3e968a64b2831d2be5156b2773fe5990b4/src/snowflake_static.rs#L21

@gliga gliga requested a review from findepi February 15, 2025 23:23
@felipecrv
Copy link
Author

Maybe link to the PR you opened to the forked repo and also to an example that led us to discover the issue, e.g., https://github.com/EngineeringSoftware/use-arrow-adbc/blob/89d83a3e968a64b2831d2be5156b2773fe5990b4/src/snowflake_static.rs#L21

And I posted the full stack trace here as well apache/arrow-adbc#2526

@findepi
Copy link
Collaborator

findepi commented Feb 16, 2025

gliga requested a review from findepi

thanks for the ping! as long as it's a backport of something that lands upstream there is no reason to have objections

the upstream PR looks simple but i have not enough context to approve or disapprove.

is the change testable?

@felipecrv
Copy link
Author

is the change testable?

I tested it with arithmetic.slt and running slt with the ADBC driver. Before the change, it panics, after the change, it passes without any problem.

I tried writing a test, but it would take a lot of work to create a RecordBatchReader in Rust that contains unaligned buffers because the factories of record batch readers take typed arrow arrays. By that point, all buffers are aligned.

@felipecrv
Copy link
Author

Discussion is evolving in apache/arrow-adbc#2526. I will merge this because this enables progress on our side while the community at large decides on the final solution.

@felipecrv felipecrv merged commit 93fa9d4 into sdf/53.3.0 Feb 17, 2025
22 of 28 checks passed
@felipecrv felipecrv deleted the felipecrv/align-ffi-buffers branch February 17, 2025 14:53
@findepi
Copy link
Collaborator

findepi commented Feb 17, 2025

I tried writing a test, but it would take a lot of work to create a RecordBatchReader in Rust that contains unaligned buffers because the factories of record batch readers take typed arrow arrays.

it it possible to create buffers from arbitrary *u8 / NonNull<u8> pointers?

@findepi
Copy link
Collaborator

findepi commented Feb 17, 2025

Discussion is evolving in apache/arrow-adbc#2526. I will merge this because this enables progress on our side while the community at large decides on the final solution.

👍

let's hope the community converges on a solution (!= wontfix)

@felipecrv
Copy link
Author

I tried writing a test, but it would take a lot of work to create a RecordBatchReader in Rust that contains unaligned buffers because the factories of record batch readers take typed arrow arrays.

it it possible to create buffers from arbitrary *u8 / NonNull<u8> pointers?

Creating the buffers is easy, instantiating the <Typed>Array -> dyn Array -> RecordBatch -> RecordBatchReader is the challenging part because as soon as you create an array the alignment is validated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants