You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/12/16 13:16:11 UTC

[GitHub] [arrow] mqy opened a new pull request #8936: ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8

mqy opened a new pull request #8936:
URL: https://github.com/apache/arrow/pull/8936


   [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: remote "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.
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] mqy commented on a change in pull request #8936: ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8

Posted by GitBox <gi...@apache.org>.
mqy commented on a change in pull request #8936:
URL: https://github.com/apache/arrow/pull/8936#discussion_r544861983



##########
File path: rust/arrow/src/ipc/convert.rs
##########
@@ -124,8 +124,11 @@ pub fn fb_to_schema(fb: ipc::Schema) -> Schema {
 
 /// Deserialize an IPC message into a schema
 pub fn schema_from_bytes(bytes: &[u8]) -> Option<Schema> {

Review comment:
       DONE




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] alamb commented on pull request #8936: ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #8936:
URL: https://github.com/apache/arrow/pull/8936#issuecomment-748962633


   > 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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] mqy commented on a change in pull request #8936: ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8

Posted by GitBox <gi...@apache.org>.
mqy commented on a change in pull request #8936:
URL: https://github.com/apache/arrow/pull/8936#discussion_r544861088



##########
File path: rust/arrow-flight/src/utils.rs
##########
@@ -113,7 +113,13 @@ pub fn flight_data_to_arrow_batch(
     schema: SchemaRef,
 ) -> Option<Result<RecordBatch>> {
     // check that the data_header is a record batch message
-    let message = arrow::ipc::get_root_as_message(&data.data_header[..]);
+    let res = arrow::ipc::root_as_message(&data.data_header[..]);
+    if res.is_err() {

Review comment:
       DONE




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] andygrove commented on a change in pull request #8936: ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8

Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #8936:
URL: https://github.com/apache/arrow/pull/8936#discussion_r544367525



##########
File path: rust/parquet/src/arrow/schema.rs
##########
@@ -184,10 +184,13 @@ fn get_arrow_schema_from_metadata(encoded_meta: &str) -> Option<Schema> {
             } else {
                 bytes.as_slice()
             };
-            let message = arrow::ipc::get_root_as_message(slice);
-            message
-                .header_as_schema()
-                .map(arrow::ipc::convert::fb_to_schema)
+            if let Ok(message) = arrow::ipc::root_as_message(slice) {

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




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] mqy commented on pull request #8936: ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8

Posted by GitBox <gi...@apache.org>.
mqy commented on pull request #8936:
URL: https://github.com/apache/arrow/pull/8936#issuecomment-746495690


   > 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 :)


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] codecov-io commented on pull request #8936: ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #8936:
URL: https://github.com/apache/arrow/pull/8936#issuecomment-747259380


   # [Codecov](https://codecov.io/gh/apache/arrow/pull/8936?src=pr&el=h1) Report
   > Merging [#8936](https://codecov.io/gh/apache/arrow/pull/8936?src=pr&el=desc) (2aa9359) into [master](https://codecov.io/gh/apache/arrow/commit/6fafedf784c531c22d12303f53fb1adfabb85355?el=desc) (6fafedf) will **decrease** coverage by `0.42%`.
   > The diff coverage is `38.02%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow/pull/8936/graphs/tree.svg?width=650&height=150&src=pr&token=LpTCFbqVT1)](https://codecov.io/gh/apache/arrow/pull/8936?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #8936      +/-   ##
   ==========================================
   - Coverage   83.26%   82.84%   -0.43%     
   ==========================================
     Files         195      196       +1     
     Lines       48066    48562     +496     
   ==========================================
   + Hits        40023    40231     +208     
   - Misses       8043     8331     +288     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/arrow/pull/8936?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [rust/arrow-flight/src/utils.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy1mbGlnaHQvc3JjL3V0aWxzLnJz) | `0.00% <0.00%> (ø)` | |
   | [rust/arrow/src/ipc/gen/SparseTensor.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2dlbi9TcGFyc2VUZW5zb3IucnM=) | `0.00% <0.00%> (ø)` | |
   | [rust/arrow/src/ipc/gen/Tensor.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2dlbi9UZW5zb3IucnM=) | `0.00% <0.00%> (ø)` | |
   | [rust/arrow/src/ipc/convert.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2NvbnZlcnQucnM=) | `91.90% <27.27%> (-1.11%)` | :arrow_down: |
   | [rust/arrow/src/ipc/gen/File.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2dlbi9GaWxlLnJz) | `40.94% <39.28%> (-2.20%)` | :arrow_down: |
   | [rust/parquet/src/arrow/schema.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9wYXJxdWV0L3NyYy9hcnJvdy9zY2hlbWEucnM=) | `91.36% <40.00%> (-0.45%)` | :arrow_down: |
   | [rust/arrow/src/ipc/gen/Message.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2dlbi9NZXNzYWdlLnJz) | `32.03% <42.10%> (+2.21%)` | :arrow_up: |
   | [rust/arrow/src/ipc/reader.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL3JlYWRlci5ycw==) | `82.86% <50.00%> (-0.84%)` | :arrow_down: |
   | [rust/arrow/src/ipc/gen/Schema.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2dlbi9TY2hlbWEucnM=) | `38.75% <62.55%> (+5.58%)` | :arrow_up: |
   | [rust/datafusion/src/execution/context.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9leGVjdXRpb24vY29udGV4dC5ycw==) | `89.95% <0.00%> (-1.93%)` | :arrow_down: |
   | ... and [5 more](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/8936?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/8936?src=pr&el=footer). Last update [6fafedf...2aa9359](https://codecov.io/gh/apache/arrow/pull/8936?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] codecov-io edited a comment on pull request #8936: ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #8936:
URL: https://github.com/apache/arrow/pull/8936#issuecomment-747259380


   # [Codecov](https://codecov.io/gh/apache/arrow/pull/8936?src=pr&el=h1) Report
   > Merging [#8936](https://codecov.io/gh/apache/arrow/pull/8936?src=pr&el=desc) (dddc1ce) into [master](https://codecov.io/gh/apache/arrow/commit/6fafedf784c531c22d12303f53fb1adfabb85355?el=desc) (6fafedf) will **decrease** coverage by `0.42%`.
   > The diff coverage is `38.02%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow/pull/8936/graphs/tree.svg?width=650&height=150&src=pr&token=LpTCFbqVT1)](https://codecov.io/gh/apache/arrow/pull/8936?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #8936      +/-   ##
   ==========================================
   - Coverage   83.26%   82.84%   -0.43%     
   ==========================================
     Files         195      196       +1     
     Lines       48066    48562     +496     
   ==========================================
   + Hits        40023    40231     +208     
   - Misses       8043     8331     +288     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/arrow/pull/8936?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [rust/arrow-flight/src/utils.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy1mbGlnaHQvc3JjL3V0aWxzLnJz) | `0.00% <0.00%> (ø)` | |
   | [rust/arrow/src/ipc/gen/SparseTensor.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2dlbi9TcGFyc2VUZW5zb3IucnM=) | `0.00% <0.00%> (ø)` | |
   | [rust/arrow/src/ipc/gen/Tensor.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2dlbi9UZW5zb3IucnM=) | `0.00% <0.00%> (ø)` | |
   | [rust/arrow/src/ipc/convert.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2NvbnZlcnQucnM=) | `91.90% <27.27%> (-1.11%)` | :arrow_down: |
   | [rust/arrow/src/ipc/gen/File.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2dlbi9GaWxlLnJz) | `40.94% <39.28%> (-2.20%)` | :arrow_down: |
   | [rust/parquet/src/arrow/schema.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9wYXJxdWV0L3NyYy9hcnJvdy9zY2hlbWEucnM=) | `91.36% <40.00%> (-0.45%)` | :arrow_down: |
   | [rust/arrow/src/ipc/gen/Message.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2dlbi9NZXNzYWdlLnJz) | `32.03% <42.10%> (+2.21%)` | :arrow_up: |
   | [rust/arrow/src/ipc/reader.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL3JlYWRlci5ycw==) | `82.86% <50.00%> (-0.84%)` | :arrow_down: |
   | [rust/arrow/src/ipc/gen/Schema.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2dlbi9TY2hlbWEucnM=) | `38.75% <62.55%> (+5.58%)` | :arrow_up: |
   | [rust/datafusion/src/execution/context.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9leGVjdXRpb24vY29udGV4dC5ycw==) | `89.95% <0.00%> (-1.93%)` | :arrow_down: |
   | ... and [5 more](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/8936?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/8936?src=pr&el=footer). Last update [6fafedf...dddc1ce](https://codecov.io/gh/apache/arrow/pull/8936?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] alamb commented on pull request #8936: ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #8936:
URL: https://github.com/apache/arrow/pull/8936#issuecomment-748525845


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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] alamb commented on pull request #8936: ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #8936:
URL: https://github.com/apache/arrow/pull/8936#issuecomment-747777230


   The conda integration test failed due to what appears to be some unrelated error: 
   ![Screen Shot 2020-12-17 at 6 49 18 PM](https://user-images.githubusercontent.com/490673/102557206-a016fd80-4098-11eb-8da4-68cf7750fbd9.png)
   
   I will restart the check to see if we can get a green CI run


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] mqy commented on a change in pull request #8936: ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8

Posted by GitBox <gi...@apache.org>.
mqy commented on a change in pull request #8936:
URL: https://github.com/apache/arrow/pull/8936#discussion_r544856382



##########
File path: rust/parquet/src/arrow/schema.rs
##########
@@ -184,10 +184,13 @@ fn get_arrow_schema_from_metadata(encoded_meta: &str) -> Option<Schema> {
             } else {
                 bytes.as_slice()
             };
-            let message = arrow::ipc::get_root_as_message(slice);
-            message
-                .header_as_schema()
-                .map(arrow::ipc::convert::fb_to_schema)
+            if let Ok(message) = arrow::ipc::root_as_message(slice) {

Review comment:
       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>>`.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] alamb closed pull request #8936: ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8

Posted by GitBox <gi...@apache.org>.
alamb closed pull request #8936:
URL: https://github.com/apache/arrow/pull/8936


   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] alamb commented on a change in pull request #8936: ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8

Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #8936:
URL: https://github.com/apache/arrow/pull/8936#discussion_r546691497



##########
File path: rust/parquet/src/arrow/schema.rs
##########
@@ -184,10 +184,21 @@ fn get_arrow_schema_from_metadata(encoded_meta: &str) -> Option<Schema> {
             } else {
                 bytes.as_slice()
             };
-            let message = arrow::ipc::get_root_as_message(slice);
-            message
-                .header_as_schema()
-                .map(arrow::ipc::convert::fb_to_schema)
+            match arrow::ipc::root_as_message(slice) {
+                Ok(message) => message
+                    .header_as_schema()
+                    .map(arrow::ipc::convert::fb_to_schema),
+                Err(err) => {
+                    // The flatbuffers implementation returns an error on verification error.

Review comment:
       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.
   
   




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] andygrove commented on pull request #8936: ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8

Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #8936:
URL: https://github.com/apache/arrow/pull/8936#issuecomment-746430232


   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.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] andygrove commented on a change in pull request #8936: ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8

Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #8936:
URL: https://github.com/apache/arrow/pull/8936#discussion_r544368023



##########
File path: rust/arrow-flight/src/utils.rs
##########
@@ -113,7 +113,13 @@ pub fn flight_data_to_arrow_batch(
     schema: SchemaRef,
 ) -> Option<Result<RecordBatch>> {
     // check that the data_header is a record batch message
-    let message = arrow::ipc::get_root_as_message(&data.data_header[..]);
+    let res = arrow::ipc::root_as_message(&data.data_header[..]);
+    if res.is_err() {

Review comment:
       Shouldn't we return `Some(Err(_))` here?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] github-actions[bot] commented on pull request #8936: ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8936:
URL: https://github.com/apache/arrow/pull/8936#issuecomment-746273042


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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] alamb commented on pull request #8936: ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #8936:
URL: https://github.com/apache/arrow/pull/8936#issuecomment-748964144


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


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] mqy commented on pull request #8936: ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8

Posted by GitBox <gi...@apache.org>.
mqy commented on pull request #8936:
URL: https://github.com/apache/arrow/pull/8936#issuecomment-747837642


   @alamb pushed another commit [0dff5ea1ee](https://github.com/apache/arrow/pull/8936/commits/0dff5ea1ee43bca90a12ab1dcb63a7dce32c8558)
   
   It's the update after fixing flatc rust generator, [google/flatbuffers 05192553](https://github.com/google/flatbuffers/commit/05192553f434d10c5f585aeb6a07a55a6ac702a5) FYI.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] mqy commented on a change in pull request #8936: ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8

Posted by GitBox <gi...@apache.org>.
mqy commented on a change in pull request #8936:
URL: https://github.com/apache/arrow/pull/8936#discussion_r544856382



##########
File path: rust/parquet/src/arrow/schema.rs
##########
@@ -184,10 +184,13 @@ fn get_arrow_schema_from_metadata(encoded_meta: &str) -> Option<Schema> {
             } else {
                 bytes.as_slice()
             };
-            let message = arrow::ipc::get_root_as_message(slice);
-            message
-                .header_as_schema()
-                .map(arrow::ipc::convert::fb_to_schema)
+            if let Ok(message) = arrow::ipc::root_as_message(slice) {

Review comment:
       DONE. Since there is already a eprintln!(), the new error is cached 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>>`.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] andygrove commented on a change in pull request #8936: ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8

Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #8936:
URL: https://github.com/apache/arrow/pull/8936#discussion_r544366618



##########
File path: rust/arrow/src/ipc/convert.rs
##########
@@ -124,8 +124,11 @@ pub fn fb_to_schema(fb: ipc::Schema) -> Schema {
 
 /// Deserialize an IPC message into a schema
 pub fn schema_from_bytes(bytes: &[u8]) -> Option<Schema> {

Review comment:
       Should we change this function to return `Result<Schema>` 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] codecov-io edited a comment on pull request #8936: ARROW-10938: [Rust] upgrade dependency "flatbuffers" to 0.8

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #8936:
URL: https://github.com/apache/arrow/pull/8936#issuecomment-747259380


   # [Codecov](https://codecov.io/gh/apache/arrow/pull/8936?src=pr&el=h1) Report
   > Merging [#8936](https://codecov.io/gh/apache/arrow/pull/8936?src=pr&el=desc) (0dff5ea) into [master](https://codecov.io/gh/apache/arrow/commit/6fafedf784c531c22d12303f53fb1adfabb85355?el=desc) (6fafedf) will **decrease** coverage by `0.41%`.
   > The diff coverage is `38.02%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow/pull/8936/graphs/tree.svg?width=650&height=150&src=pr&token=LpTCFbqVT1)](https://codecov.io/gh/apache/arrow/pull/8936?src=pr&el=tree)
   
   ```diff
   @@            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](https://codecov.io/gh/apache/arrow/pull/8936?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [rust/arrow-flight/src/utils.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy1mbGlnaHQvc3JjL3V0aWxzLnJz) | `0.00% <0.00%> (ø)` | |
   | [rust/arrow/src/ipc/gen/SparseTensor.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2dlbi9TcGFyc2VUZW5zb3IucnM=) | `0.00% <0.00%> (ø)` | |
   | [rust/arrow/src/ipc/gen/Tensor.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2dlbi9UZW5zb3IucnM=) | `0.00% <0.00%> (ø)` | |
   | [rust/arrow/src/ipc/convert.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2NvbnZlcnQucnM=) | `91.90% <27.27%> (-1.11%)` | :arrow_down: |
   | [rust/arrow/src/ipc/gen/File.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2dlbi9GaWxlLnJz) | `40.94% <39.28%> (-2.20%)` | :arrow_down: |
   | [rust/parquet/src/arrow/schema.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9wYXJxdWV0L3NyYy9hcnJvdy9zY2hlbWEucnM=) | `91.36% <40.00%> (-0.45%)` | :arrow_down: |
   | [rust/arrow/src/ipc/gen/Message.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2dlbi9NZXNzYWdlLnJz) | `32.03% <42.10%> (+2.21%)` | :arrow_up: |
   | [rust/arrow/src/ipc/reader.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL3JlYWRlci5ycw==) | `82.86% <50.00%> (-0.84%)` | :arrow_down: |
   | [rust/arrow/src/ipc/gen/Schema.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2dlbi9TY2hlbWEucnM=) | `38.75% <62.55%> (+5.58%)` | :arrow_up: |
   | [rust/datafusion/src/execution/context.rs](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9leGVjdXRpb24vY29udGV4dC5ycw==) | `89.95% <0.00%> (-1.93%)` | :arrow_down: |
   | ... and [10 more](https://codecov.io/gh/apache/arrow/pull/8936/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/8936?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/8936?src=pr&el=footer). Last update [6fafedf...0dff5ea](https://codecov.io/gh/apache/arrow/pull/8936?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org