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

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

Closed
wants to merge 1 commit into from

Conversation

mqy
Copy link
Contributor

@mqy mqy commented Jan 12, 2021

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

@mqy mqy marked this pull request as draft January 12, 2021 13:57
@mqy
Copy link
Contributor Author

mqy commented Jan 12, 2021

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
Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @mqy

@alamb
Copy link
Contributor

alamb commented Jan 13, 2021

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
Copy link
Contributor Author

mqy commented Jan 15, 2021

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
Copy link
Contributor

alamb commented Jan 17, 2021

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
Copy link
Contributor Author

mqy commented Jan 17, 2021

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
Copy link
Contributor

alamb commented Jan 19, 2021

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>
GeorgeAp pushed a commit to sirensolutions/arrow that referenced this pull request Jun 7, 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 apache#9176 from mqy/flatbuffers-0.8.1

Authored-by: mqy <meng.qingyou@gmail.com>
Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>
michalursa pushed a commit to michalursa/arrow that referenced this pull request Jun 13, 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 apache#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