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/09/12 15:33:24 UTC
[GitHub] [arrow] nevi-me opened a new pull request #8174: ARROW-9848: [Rust] Implement 0.15 IPC alignment
nevi-me opened a new pull request #8174:
URL: https://github.com/apache/arrow/pull/8174
Changes introduced in 0.15.0 changed the buffer alignment by adding a continuation marker to messages.
The previous behaviour was then marked as legacy, while both worked under V4 of the IPC metadata version.
This change catches the Rust implementation up to other languages, and has the consequence that more integration tests now pass.
The change is applied on top of clippy changes, and can be reviewed/merged after the relevant PRs.
----------------------------------------------------------------
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] nevi-me commented on pull request #8174: ARROW-9848: [Rust] Implement 0.15 IPC alignment
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #8174:
URL: https://github.com/apache/arrow/pull/8174#issuecomment-692401838
@andygrove @paddyhoran @sunchao the changes here still work with the 0.14.x golden files, and 0.15+ as integration tests pass. Would you like to perform an in-depth review, or can I merge the changes? I have a follow-up that enables more tests, which I'd like to try wrap up over/before the weekend
----------------------------------------------------------------
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] nevi-me commented on pull request #8174: ARROW-9848: [Rust] Implement 0.15 IPC alignment
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #8174:
URL: https://github.com/apache/arrow/pull/8174#issuecomment-691506881
@maxburke this will likely affect you, please have a look at it before the next release if you can. The Rust writer wasn't writing continuation markers consistently, and this is fixed. Readers should be able to continue reading, but it's worth testing out with your workloads.
----------------------------------------------------------------
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 closed pull request #8174: ARROW-9848: [Rust] Implement 0.15 IPC alignment
Posted by GitBox <gi...@apache.org>.
andygrove closed pull request #8174:
URL: https://github.com/apache/arrow/pull/8174
----------------------------------------------------------------
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] nevi-me commented on pull request #8174: ARROW-9848: [Rust] Implement 0.15 IPC alignment
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #8174:
URL: https://github.com/apache/arrow/pull/8174#issuecomment-691742334
@wesm I found the problem, I was setting children as null instead of an empty list, so not a library issue. Thanks again
----------------------------------------------------------------
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] nevi-me commented on pull request #8174: ARROW-9848: [Rust] Implement 0.15 IPC alignment
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #8174:
URL: https://github.com/apache/arrow/pull/8174#issuecomment-691506881
----------------------------------------------------------------
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 #8174: ARROW-9848: [Rust] Implement 0.15 IPC alignment
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8174:
URL: https://github.com/apache/arrow/pull/8174#issuecomment-691508247
----------------------------------------------------------------
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] nevi-me commented on pull request #8174: ARROW-9848: [Rust] Implement 0.15 IPC alignment
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #8174:
URL: https://github.com/apache/arrow/pull/8174#issuecomment-691728298
Thanks Wes, from looking at the cpp source where the error occurs, the 0-length case makes me wonder if this might be a fbs issue in Rust. I'll have a look at whether the Rust impl distinguishes between a null and empty array value.
Might have to set a value then remove it (if possible) to ensure that there's an empty array.
----------------------------------------------------------------
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 #8174: ARROW-9848: [Rust] Implement 0.15 IPC alignment
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8174:
URL: https://github.com/apache/arrow/pull/8174#issuecomment-691508247
https://issues.apache.org/jira/browse/ARROW-9848
----------------------------------------------------------------
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] nevi-me commented on pull request #8174: ARROW-9848: [Rust] Implement 0.15 IPC alignment
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #8174:
URL: https://github.com/apache/arrow/pull/8174#issuecomment-691506881
@maxburke this will likely affect you, please have a look at it before the next release if you can. The Rust writer wasn't writing continuation markers consistently, and this is fixed. Readers should be able to continue reading, but it's worth testing out with your workloads.
----------------------------------------------------------------
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] maxburke commented on pull request #8174: ARROW-9848: [Rust] Implement 0.15 IPC alignment
Posted by GitBox <gi...@apache.org>.
maxburke commented on pull request #8174:
URL: https://github.com/apache/arrow/pull/8174#issuecomment-692785370
> @maxburke this will likely affect you, please have a look at it before the next release if you can. The Rust writer wasn't writing continuation markers consistently, and this is fixed. Readers should be able to continue reading, but it's worth testing out with your workloads.
Thanks for the heads-up, @nevi-me, we'll give it a solid runthrough the next time we pull.
----------------------------------------------------------------
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] nevi-me commented on pull request #8174: ARROW-9848: [Rust] Implement 0.15 IPC alignment
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #8174:
URL: https://github.com/apache/arrow/pull/8174#issuecomment-691617589
I'll look into the integration failures during the week, but it's C++ null arrays. I noticed similar failures when I was trying to read a null array from pyarrow. Seems I'm likely not understanding the spec correctly (odd that C++ is the only failure when Java and others pass).
----------------------------------------------------------------
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] wesm commented on pull request #8174: ARROW-9848: [Rust] Implement 0.15 IPC alignment
Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #8174:
URL: https://github.com/apache/arrow/pull/8174#issuecomment-691727701
In C++ we are more hardened against metadata integrity issues from all the IPC fuzzing, e.g.
https://github.com/apache/arrow/blob/master/cpp/src/arrow/ipc/metadata_internal.cc#L747
According to the Flatbuffers spec, this field should be length-0, not null.
----------------------------------------------------------------
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] nevi-me commented on pull request #8174: ARROW-9848: [Rust] Implement 0.15 IPC alignment
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #8174:
URL: https://github.com/apache/arrow/pull/8174#issuecomment-691506881
----------------------------------------------------------------
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] nevi-me commented on a change in pull request #8174: ARROW-9848: [Rust] Implement 0.15 IPC alignment
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #8174:
URL: https://github.com/apache/arrow/pull/8174#discussion_r487448427
##########
File path: rust/arrow/src/ipc/mod.rs
##########
@@ -36,3 +36,4 @@ pub use self::gen::SparseTensor::*;
pub use self::gen::Tensor::*;
static ARROW_MAGIC: [u8; 6] = [b'A', b'R', b'R', b'O', b'W', b'1'];
Review comment:
is `const` better than `static` here? IIRC I used static for `ARROW_MAGIC` when const either wasn't a thing yet, or I was unfamiliar with it.
##########
File path: rust/arrow/src/ipc/mod.rs
##########
@@ -36,3 +36,4 @@ pub use self::gen::SparseTensor::*;
pub use self::gen::Tensor::*;
static ARROW_MAGIC: [u8; 6] = [b'A', b'R', b'R', b'O', b'W', b'1'];
Review comment:
is `const` better than `static` here? IIRC I used static for `ARROW_MAGIC` when const either wasn't a thing yet, or I was unfamiliar with it.
----------------------------------------------------------------
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] paddyhoran commented on a change in pull request #8174: ARROW-9848: [Rust] Implement 0.15 IPC alignment
Posted by GitBox <gi...@apache.org>.
paddyhoran commented on a change in pull request #8174:
URL: https://github.com/apache/arrow/pull/8174#discussion_r488682274
##########
File path: rust/arrow/src/ipc/mod.rs
##########
@@ -36,3 +36,4 @@ pub use self::gen::SparseTensor::*;
pub use self::gen::Tensor::*;
static ARROW_MAGIC: [u8; 6] = [b'A', b'R', b'R', b'O', b'W', b'1'];
Review comment:
Yea, I think this can be `const`.
----------------------------------------------------------------
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 #8174: ARROW-9848: [Rust] Implement 0.15 IPC alignment
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8174:
URL: https://github.com/apache/arrow/pull/8174#issuecomment-691508247
----------------------------------------------------------------
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