Skip to content

ARROW-10461: [Rust] Fix offset bug in remainder bits - #8571

Closed
jhorstmann wants to merge 6 commits into
apache:masterfrom
jhorstmann:ARROW-10461-offset-bug-in-remainder-bits
Closed

ARROW-10461: [Rust] Fix offset bug in remainder bits#8571
jhorstmann wants to merge 6 commits into
apache:masterfrom
jhorstmann:ARROW-10461-offset-bug-in-remainder-bits

Conversation

@jhorstmann

@jhorstmannjhorstmann commented Nov 2, 2020

Copy link
Copy Markdown
Contributor

No description provided.

@github-actions

github-actionsBot commented Nov 2, 2020

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-10461

svenwb
svenwb approved these changes Nov 2, 2020
View reviewed changes

@svenwbsvenwb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

vertexclique
vertexclique suggested changes Nov 3, 2020
View reviewed changes

@vertexcliquevertexclique left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to have documentation for the whole code and make it understandable. Including the code that is using this in the Array method call, tests, expectations, and briefly what it does.

Comment threadrust/arrow/src/util/bit_chunk_iterator.rs
Comment threadrust/arrow/src/util/bit_chunk_iterator.rs
Comment threadrust/arrow/src/util/bit_chunk_iterator.rs Outdated

assert_eq!(63, bitchunks.remainder_len());
assert_eq!(
0b01000000_00111111_11000000_00111111_11000000_00111111_11000000_00111111,

@vertexcliquevertexcliqueNov 3, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this forms these bit pattern? How a reader can be ensure that it is forming the correct pattern while reading?

Comment threadrust/arrow/src/util/bit_chunk_iterator.rs
@jhorstmann

jhorstmann commented Nov 5, 2020

Copy link
Copy Markdown
ContributorAuthor

@jorgecarleitao@nevi-me Can you take a look at this bugfix?

@jorgecarleitao

jorgecarleitao commented Nov 6, 2020

Copy link
Copy Markdown
Member

@jhorstmann , bit operations is one of my weaknesses; I can't review this due to my lack of knowledge on that topic.

@alambalamb mentioned this pull request Nov 6, 2020
Closed
alamb
alamb approved these changes Nov 6, 2020
View reviewed changes

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. 👍 for the tests

Comment threadrust/arrow-flight/Cargo.toml Outdated
[dependencies]
arrow = { path = "../arrow", version = "3.0.0-SNAPSHOT" }
tonic = "0.3"
tonic = "0.3.1"

@alambalambNov 6, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there is some reason to upgrade tonic in this same PR?

@jhorstmannjhorstmannNov 6, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had compilation failures otherwise, but it's strange that other PRs do not seem to have the same issue. I'll check again and maybe move this to a separate PR.

@jhorstmannjhorstmannNov 6, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the dependency again and CI still works, not sure what the issue was locally.

Comment threadrust/arrow/src/util/bit_chunk_iterator.rs
@alamb

alamb commented Nov 7, 2020

Copy link
Copy Markdown
Contributor

FYI @jorgecarleitao / @nevi-me I think we should merge this PR.

@jhorstmann

jhorstmann commented Nov 10, 2020

Copy link
Copy Markdown
ContributorAuthor

@nevi-me@alamb@jorgecarleitao I had a quick chat with @vertexclique and we think we can merge this PR with the bugfix first and then rebase and integrate his refactoring. Those PRs shouldn't block each other.

@alamb

alamb commented Nov 10, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

I agree @jhorstmann -- I think we should merge this. I don't yet have the permissions set up to merge things, so I need to wait for one of the other committer such as @andygrove , @nevi-me or @jorgecarleitao to do so

Update: the bits came through and I have merged this PR. FYI @andygrove@nevi-me and @jorgecarleitao

@alambalamb closed this in 41d2438Nov 10, 2020
@jorgecarleitao

jorgecarleitao commented Nov 10, 2020

Copy link
Copy Markdown
Member

@jhorstmann thanks a lot for this fix!

@alamb, congrats on your push!!!!! 🎉 🎉 🎉 🎉 🎉

@asfimportasfimport mentioned this pull request Nov 10, 2020
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alambalambalamb approved these changes
@nevi-menevi-menevi-me approved these changes
+2 more reviewers
@vertexcliquevertexcliquevertexclique requested changes
@svenwbsvenwbsvenwb approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Component: Rust

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jhorstmann@jorgecarleitao@alamb@vertexclique@nevi-me@svenwb