ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8 #8936

mqy · 2020-12-16T13:16:00Z

flatbuffers 0.8.0 was released on Dec 10, 2020, with some notable changes:

new verifier
common rust traits to FlatBufferBuilder
new VectorIter
add FlatBufferBuilder::force_defaults API
Optional Scalars
up to 2018 edition
possible performance speedup
... and minor breaking change to some APIs, for example: remove "get_", return Result.

flatbuffers 0.8.0 requires the latest flatc, the git commit for flatc is updated too.

I deliberately commit all changes step by step to make them clear.

… rust/arrow/src/ipc/gen/ format/*.fbs

…sed by API changes

github-actions · 2020-12-16T13:18:03Z

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

rust/arrow/src/ipc/convert.rs

andygrove · 2020-12-16T14:59:32Z

rust/parquet/src/arrow/schema.rs

-            message
-                .header_as_schema()
-                .map(arrow::ipc::convert::fb_to_schema)
+            if let Ok(message) = arrow::ipc::root_as_message(slice) {


We are ignoring errors here too. Is this the intent?

DONE. Since there is already a eprintln!(), the new error is caught with eprintln!() too.
We may improve them later, right?

BTW, I failed to change the return type of get_arrow_schema_from_metadata() from Option<Schema> to Result<Schema> or Result<Option<Schema>>.

rust/arrow-flight/src/utils.rs

andygrove · 2020-12-16T15:00:51Z

Thanks @mqy this is looking good and I appreciate you breaking this down into the individual commits. I have some questions on error handling but other than that it LGTM.

mqy · 2020-12-16T15:40:03Z

Thanks @mqy this is looking good and I appreciate you breaking this down into the individual commits. I have some questions on error handling but other than that it LGTM.

@andygrove that's great! Since i'm pretty new to rust, it may take me some time to completely fix/enhance the "Option -> Result problems", and time to sleep for now.

I'll continue this PR next morning, if ... it is not merged :)

codecov-io · 2020-12-17T07:14:17Z

Codecov Report

Merging #8936 (0dff5ea) into master (6fafedf) will decrease coverage by 0.41%.
The diff coverage is 38.02%.

@@            Coverage Diff             @@
##           master    #8936      +/-   ##
==========================================
- Coverage   83.26%   82.85%   -0.42%     
==========================================
  Files         195      196       +1     
  Lines       48066    48577     +511     
==========================================
+ Hits        40023    40249     +226     
- Misses       8043     8328     +285

Impacted Files	Coverage Δ
rust/arrow-flight/src/utils.rs	`0.00% <0.00%> (ø)`
rust/arrow/src/ipc/gen/SparseTensor.rs	`0.00% <0.00%> (ø)`
rust/arrow/src/ipc/gen/Tensor.rs	`0.00% <0.00%> (ø)`
rust/arrow/src/ipc/convert.rs	`91.90% <27.27%> (-1.11%)`	⬇️
rust/arrow/src/ipc/gen/File.rs	`40.94% <39.28%> (-2.20%)`	⬇️
rust/parquet/src/arrow/schema.rs	`91.36% <40.00%> (-0.45%)`	⬇️
rust/arrow/src/ipc/gen/Message.rs	`32.03% <42.10%> (+2.21%)`	⬆️
rust/arrow/src/ipc/reader.rs	`82.86% <50.00%> (-0.84%)`	⬇️
rust/arrow/src/ipc/gen/Schema.rs	`38.75% <62.55%> (+5.58%)`	⬆️
rust/datafusion/src/execution/context.rs	`89.95% <0.00%> (-1.93%)`	⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fafedf...0dff5ea. Read the comment docs.

alamb · 2020-12-17T23:50:00Z

The conda integration test failed due to what appears to be some unrelated error:

I will restart the check to see if we can get a green CI run

… flatc always

mqy · 2020-12-18T03:02:19Z

@alamb pushed another commit 0dff5ea1ee

It's the update after fixing flatc rust generator, google/flatbuffers 05192553 FYI.

nevi-me

LGTM, we should open a JIRA to move us to the next flatbuffers release

alamb · 2020-12-19T21:14:08Z

Thanks @mqy -- I'll try and look at this carefully tomorrow.

alamb · 2020-12-21T13:03:11Z

LGTM, we should open a JIRA to move us to the next flatbuffers release

@nevi-me -- filed https://issues.apache.org/jira/browse/ARROW-10997

alamb

I went through this commit by commit -- the breakdown of this PR into commits was super helpful. I'll file some tickets to handle follow on improvements but then I plan to merge this one in.

Thanks again @mqy

alamb · 2020-12-21T12:57:42Z

rust/parquet/src/arrow/schema.rs

+                    .header_as_schema()
+                    .map(arrow::ipc::convert::fb_to_schema),
+                Err(err) => {
+                    // The flatbuffers implementation returns an error on verification error.


I filed https://issues.apache.org/jira/browse/ARROW-10996 to track returning this is a proper error message. I think this PR improves the situation so at least eprintln! is called where prior to this PR no error message is created.

alamb · 2020-12-21T13:06:50Z

I merged this branch into apache/master and ran the tests locally just to be sure things are good. Merging

…chema_from_metadata() #8936 updated crate `flatbuffers` to 0.8.0 , but function `get_arrow_schema_from_metadata` still returning `Option` rather than `Result`. This PR fixes this issue. Closes #9058 from mqy/get_arrow_schema_from_metadata Authored-by: mqy <meng.qingyou@gmail.com> Signed-off-by: Jorge C. Leitao <jorgecarleitao@gmail.com>

…chema_from_metadata() apache/arrow#8936 updated crate `flatbuffers` to 0.8.0 , but function `get_arrow_schema_from_metadata` still returning `Option` rather than `Result`. This PR fixes this issue. Closes #9058 from mqy/get_arrow_schema_from_metadata Authored-by: mqy <meng.qingyou@gmail.com> Signed-off-by: Jorge C. Leitao <jorgecarleitao@gmail.com>

[flatbuffers](https://crates.io/crates/flatbuffers) 0.8.0 was released on Dec 10, 2020, with some notable changes: - new verifier - common rust traits to FlatBufferBuilder - new VectorIter - add FlatBufferBuilder::force_defaults API - Optional Scalars - up to 2018 edition - possible performance speedup - ... and minor breaking change to some APIs, for example: remove "get_", return Result. flatbuffers 0.8.0 requires the latest flatc, the git commit for flatc is updated too. I deliberately commit all changes step by step to make them clear. Closes apache#8936 from mqy/flatbuffers-0.8.0 Authored-by: mqy <meng.qingyou@gmail.com> Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>

…chema_from_metadata() apache#8936 updated crate `flatbuffers` to 0.8.0 , but function `get_arrow_schema_from_metadata` still returning `Option` rather than `Result`. This PR fixes this issue. Closes apache#9058 from mqy/get_arrow_schema_from_metadata Authored-by: mqy <meng.qingyou@gmail.com> Signed-off-by: Jorge C. Leitao <jorgecarleitao@gmail.com>

mqy added 4 commits December 16, 2020 19:33

regen with flatc @ commit f8b203c9: flatc --filename-suffix --rust -o…

514b0d6

… rust/arrow/src/ipc/gen/ format/*.fbs

After various sed replacement, before cargo fmt

35dc55e

After cargo +stable fmt -- src/ipc/gen/*

f4c17ad

Add new regen.sh, bump flatbuffers crate to 0.8, fix build errors cau…

e5833b7

…sed by API changes

github-actions bot added Component: FlightRPC Component: Rust Component: Parquet labels Dec 16, 2020

nevi-me self-requested a review December 16, 2020 13:48

andygrove reviewed Dec 16, 2020

View reviewed changes

rust/arrow/src/ipc/convert.rs Outdated Show resolved Hide resolved

andygrove reviewed Dec 16, 2020

View reviewed changes

rust/arrow-flight/src/utils.rs Outdated Show resolved Hide resolved

Catch errors, thanks to andygrove

2aa9359

No longer rename *_generated.rs

dddc1ce

mqy requested a review from andygrove December 17, 2020 09:57

regen.sh: update flatc to commit 05192553, remove '_generated', build…

0dff5ea

… flatc always

nevi-me approved these changes Dec 19, 2020

View reviewed changes

alamb approved these changes Dec 21, 2020

View reviewed changes

alamb closed this in f52a100 Dec 21, 2020

mqy mentioned this pull request Dec 31, 2020

ARROW-10996: [Rust] [Parquet] change return value type of get_arrow_schema_from_metadata() #9058

Closed

alamb mentioned this pull request Apr 26, 2021

upgrade dependency "flatbuffers" to release after 0.8 apache/arrow-rs#156

Closed

This was referenced Dec 25, 2020

[Rust] upgrade dependency "flatbuffers" to 0.8 #26865

Closed

[Rust] upgrade dependency "flatbuffers" to release after 0.8 #26918

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8 #8936

ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8 #8936

mqy commented Dec 16, 2020 •

edited

github-actions bot commented Dec 16, 2020

andygrove Dec 16, 2020

mqy Dec 17, 2020 •

edited

andygrove commented Dec 16, 2020

mqy commented Dec 16, 2020

codecov-io commented Dec 17, 2020 •

edited

alamb commented Dec 17, 2020

mqy commented Dec 18, 2020

nevi-me left a comment

alamb commented Dec 19, 2020

alamb commented Dec 21, 2020

alamb left a comment

alamb Dec 21, 2020

alamb commented Dec 21, 2020

ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8 #8936

ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8 #8936

Conversation

mqy commented Dec 16, 2020 • edited

github-actions bot commented Dec 16, 2020

andygrove Dec 16, 2020

Choose a reason for hiding this comment

mqy Dec 17, 2020 • edited

Choose a reason for hiding this comment

andygrove commented Dec 16, 2020

mqy commented Dec 16, 2020

codecov-io commented Dec 17, 2020 • edited

Codecov Report

alamb commented Dec 17, 2020

mqy commented Dec 18, 2020

nevi-me left a comment

Choose a reason for hiding this comment

alamb commented Dec 19, 2020

alamb commented Dec 21, 2020

alamb left a comment

Choose a reason for hiding this comment

alamb Dec 21, 2020

Choose a reason for hiding this comment

alamb commented Dec 21, 2020

mqy commented Dec 16, 2020 •

edited

mqy Dec 17, 2020 •

edited

codecov-io commented Dec 17, 2020 •

edited