You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "itsjunetime (via GitHub)" <gi...@apache.org> on 2024/11/08 23:08:35 UTC
[PR] Fix Buffer::bit_slice losing length with byte-aligned offsets [arrow-rs]
itsjunetime opened a new pull request, #6707:
URL: https://github.com/apache/arrow-rs/pull/6707
# Which issue does this PR close?
A part of #3478; necessary for #6690, which closes the aforementioned issue.
# Rationale for this change
If `bit_slice` is called with a given length, the returned buffer should have the specified length.
# What changes are included in this PR?
The `bit_slice` function itself is updated with the fix, along with a unit test to ensure the fix works. I've ensured the test fails without my changes.
I also changed the 'minimum overhead' value in an encoding test that went down due to this change.
This also updates the MSRV of this crate in Cargo.toml to `1.75`, as that was (and still is, with this PR) the effective MSRV of these crates (as found by `cargo-msrv`). This was necessary since I wanted to use `usize::div_ceil`, and that was stabilized in 1.73, but clippy was complaining that I couldn't use it since the crate's msrv was 1.62.
# Are there any user-facing changes?
Yes, a bug fix that could change user behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix Buffer::bit_slice losing length with byte-aligned offsets [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #6707:
URL: https://github.com/apache/arrow-rs/pull/6707
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix Buffer::bit_slice losing length with byte-aligned offsets [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6707:
URL: https://github.com/apache/arrow-rs/pull/6707#discussion_r1844531169
##########
.github/workflows/rust.yml:
##########
@@ -122,8 +122,6 @@ jobs:
uses: ./.github/actions/setup-builder
- name: Install cargo-msrv
run: cargo install cargo-msrv
- - name: Downgrade arrow dependencies
Review Comment:
This seems to have been added as part of https://github.com/apache/arrow-rs/pull/5385
##########
Cargo.toml:
##########
@@ -74,7 +74,7 @@ include = [
"Cargo.toml",
]
edition = "2021"
-rust-version = "1.62"
+rust-version = "1.70"
Review Comment:
I am sorry for being dense but I still don't understand this change
> I made this change because that is the effective MSRV of the majority of the crates in this workspace. It was outdated and when I disabled rust-version requirements and ran cargo msrv find in each of the crates in this repo, these were the values I found.
Since the `msrv` CI check is passing on main
https://github.com/apache/arrow-rs/actions/runs/11862785735/job/33062922708
Does that mean the CI check is not running accurately?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix Buffer::bit_slice losing length with byte-aligned offsets [arrow-rs]
Posted by "itsjunetime (via GitHub)" <gi...@apache.org>.
itsjunetime commented on PR #6707:
URL: https://github.com/apache/arrow-rs/pull/6707#issuecomment-2480863887
I've removed the MSRV changes - I thought that was going to be a simple change initially, then it ended up branching out to a lot of other things. I'm going to make a separate issue and PR here to fix that specifically.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix Buffer::bit_slice losing length with byte-aligned offsets [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6707:
URL: https://github.com/apache/arrow-rs/pull/6707#discussion_r1837835062
##########
Cargo.toml:
##########
@@ -74,7 +74,7 @@ include = [
"Cargo.toml",
]
edition = "2021"
-rust-version = "1.62"
+rust-version = "1.70"
Review Comment:
Why this change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix Buffer::bit_slice losing length with byte-aligned offsets [arrow-rs]
Posted by "itsjunetime (via GitHub)" <gi...@apache.org>.
itsjunetime commented on PR #6707:
URL: https://github.com/apache/arrow-rs/pull/6707#issuecomment-2465907322
Ah, it looks like `cargo msrv find` tries to find the minimum available version that would work for _all_ the crates in your workspace, including e.g. the parquet bin target and its `clap` dependency. I'll fix that and use `bit_util::ceil` instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix Buffer::bit_slice losing length with byte-aligned offsets [arrow-rs]
Posted by "itsjunetime (via GitHub)" <gi...@apache.org>.
itsjunetime commented on code in PR #6707:
URL: https://github.com/apache/arrow-rs/pull/6707#discussion_r1838616257
##########
Cargo.toml:
##########
@@ -74,7 +74,7 @@ include = [
"Cargo.toml",
]
edition = "2021"
-rust-version = "1.62"
+rust-version = "1.70"
Review Comment:
I made this change because that is the effective MSRV of the majority of the crates in this workspace. It was outdated and when I disabled `rust-version` requirements and ran `cargo msrv find` in each of the crates in this repo, these were the values I found.
I know it's not directly related to what this PR was intended for, but since it tripped me up when I tried to use something outside the MRSV, I figured it was worth the time to go through each crate and make sure the MSRVs are actually accurate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org