Skip to content

ARROW-11222: [Rust] Catch up with flatbuffers 0.8.1 which had some UB problems fixed#9176

Closed
mqy wants to merge 1 commit into
apache:masterfrom
mqy:flatbuffers-0.8.1
Closed

ARROW-11222: [Rust] Catch up with flatbuffers 0.8.1 which had some UB problems fixed#9176
mqy wants to merge 1 commit into
apache:masterfrom
mqy:flatbuffers-0.8.1

Conversation

@mqy

@mqy mqy commented Jan 12, 2021

Copy link
Copy Markdown
Contributor

The major change of flatbuffers 0.8.1 since 0.8.0 is google/flatbuffers#6393, which fixed some possible memory alignment issues.

In this PR, the ipc/gen/*.rs files are generated by regen.sh as before, without any manual change.

@github-actions

Copy link
Copy Markdown

@mqy mqy marked this pull request as draft January 12, 2021 13:57
@mqy

mqy commented Jan 12, 2021

Copy link
Copy Markdown
Contributor Author

The crate flatbuffers is not pinned to 0.8.1, so let's run cargo update to pull it locally.

@mqy mqy marked this pull request as ready for review January 12, 2021 14:32

@andygrove andygrove left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks @mqy

@alamb

alamb commented Jan 13, 2021

Copy link
Copy Markdown
Contributor

I suggest we hold off merging this PR until the Arrow 3.0 release is completed (happening in the next day or two) to minimize the chance we break something right before the release

@mqy

mqy commented Jan 15, 2021

Copy link
Copy Markdown
Contributor Author

I suggest we hold off merging this PR until the Arrow 3.0 release is completed (happening in the next day or two) to minimize the chance we break something right before the release

@alamb thanks for the review! Since there is no direct evidence that this PR can fix something in arrow, it's OK to me to hold off merging this PR.

BTW, I've exploring the cause of test_struct test failure for several days, filed several issues, but can't figure out the exact reason. The latest issue is filed and commented at https://issues.apache.org/jira/projects/ARROW/issues/ARROW-11239. It looks like ArrayRef::slice() is not bug-free. I suggest you evaluate this problem seriously before the 3.0 release.

CC @jorgecarleitao I'm glad if you can take time for this possible problem at this weekend, as you said :)

@nevi-me nevi-me changed the title ARROW-11222: [Rust] [Arrow] Catch up with flatbuffers 0.8.1 which had some UB problems fixed ARROW-11222: [Rust] Catch up with flatbuffers 0.8.1 which had some UB problems fixed Jan 16, 2021
@alamb

alamb commented Jan 17, 2021

Copy link
Copy Markdown
Contributor

I apologize for the delay in merging Rust PRs -- the 3.0 release is being finalized now and are planning to minimize entropy by postponing merging changes not critical for the release until the process was complete. I hope the process is complete in the next few days. There is more discussion in the mailing list

@mqy

mqy commented Jan 17, 2021

Copy link
Copy Markdown
Contributor Author

I apologize for the delay in merging Rust PRs

@alamb I understand the common releasing process that freezes pushing/merging unless for those fully tested or fatal bug fix, this is really important. You are welcome, no need to apologize at all 👍

@alamb

alamb commented Jan 19, 2021

Copy link
Copy Markdown
Contributor

I merged this branch (locally) to master and re-ran the tests to verify things still work. Everything still looks good, so merging it in.

@alamb alamb closed this in 35053fe Jan 19, 2021
kszucs pushed a commit that referenced this pull request Jan 25, 2021
… problems fixed

The major change of [flatbuffers 0.8.1](https://docs.rs/flatbuffers/0.8.1/flatbuffers/index.html) since 0.8.0 is google/flatbuffers#6393, which fixed some possible memory alignment issues.

In this PR, the ipc/gen/*.rs files are generated by `regen.sh` as before, without any manual change.

Closes #9176 from mqy/flatbuffers-0.8.1

Authored-by: mqy <meng.qingyou@gmail.com>
Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants