-
Notifications
You must be signed in to change notification settings - Fork 839
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
Switch to use u64 range instead of usize, for better wasm32 support #6961
base: main
Are you sure you want to change the base?
Conversation
The CI failure https://github.com/apache/arrow-rs/actions/runs/12713696201/job/35442156669?pr=6961 should be fixed with this PR: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @XiangpengHao ❤️
Can you help me understand what currently doesn't work with 32 bit builds now?
We have an existing test for WASM32 that seems to work fine: https://github.com/apache/arrow-rs/actions/runs/12680096825/job/35341184364
I also locally tried checking with an i686 target and that worked fine too 🤔
rustup target add i686-unknown-linux-gnu
cd object_store
cargo check --target=i686-unknown-linux-gnu
...
🤔
🤔 maybe it is related to wanting to use the http features? But it still seems to compile just fine for me 🤔
|
For anyone following along, the answer is here: #5351 (comment) I still think we should try and add some sort of test that would fail on wasm32 before this change and not after the change. If we don't do that I feel like:
I'll see if I can find some time over the next day or two to help, if no on ebeats me to it |
Which issue does this PR close?
usize
withu64
#5351Rationale for this change
Ranges should be u64 so that 32 bit platforms can read files larger than 4GB. More details can be found in #5351
What changes are included in this PR?
This is a rather simple change. No functionality change for 64 bit platforms. But is a breaking change for trait implementors.
Given that we already break one in #6619, it's seems like a good timing to also include this change.
This PR added and removed a few casting. I have checked the casting is ok, but please help me check again.
My principle of using
usize
vsu64
:usize
, e.g., slicing a memory rangeu64
Are there any user-facing changes?