ARROW-9848: [Rust] Implement 0.15 IPC alignment by nevi-me · Pull Request #8174 · apache/arrow

nevi-me · 2020-09-12T15:33:12Z

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.

nevi-me · 2020-09-12T15:34:50Z

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

github-actions · 2020-09-12T15:46:40Z

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

nevi-me · 2020-09-12T21:00:46Z

@@ -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'];


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.

Yea, I think this can be const.

nevi-me · 2020-09-13T05:36:01Z

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

wesm · 2020-09-13T21:25:35Z

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.

nevi-me · 2020-09-13T21:29:54Z

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.

Children should not be None/null, but should be an empty list

nevi-me · 2020-09-13T23:38:39Z

@wesm I found the problem, I was setting children as null instead of an empty list, so not a library issue. Thanks again

nevi-me · 2020-09-15T01:15:37Z

@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

paddyhoran

I didn't do an in-depth review but from a high level this looks good. Let's merge this and keep the momentum going. Great job @nevi-me

paddyhoran · 2020-09-15T13:48:29Z

@@ -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'];


Yea, I think this can be const.

maxburke · 2020-09-15T15:16:24Z

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

nevi-me · 2020-09-12T15:33:12Z

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.

nevi-me · 2020-09-12T15:34:50Z

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

github-actions · 2020-09-12T15:46:40Z

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

nevi-me · 2020-09-12T21:00:46Z

@@ -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'];


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.

Yea, I think this can be const.

nevi-me · 2020-09-13T05:36:01Z

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

wesm · 2020-09-13T21:25:35Z

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.

nevi-me · 2020-09-13T21:29:54Z

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.

Children should not be None/null, but should be an empty list

nevi-me · 2020-09-13T23:38:39Z

@wesm I found the problem, I was setting children as null instead of an empty list, so not a library issue. Thanks again

nevi-me · 2020-09-15T01:15:37Z

@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

paddyhoran

I didn't do an in-depth review but from a high level this looks good. Let's merge this and keep the momentum going. Great job @nevi-me

paddyhoran · 2020-09-15T13:48:29Z

@@ -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'];


Yea, I think this can be const.

maxburke · 2020-09-15T15:16:24Z

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

nevi-me · 2020-09-12T15:33:12Z

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.

nevi-me · 2020-09-12T15:34:50Z

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

github-actions · 2020-09-12T15:46:40Z

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

nevi-me · 2020-09-12T21:00:46Z

@@ -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'];


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.

Yea, I think this can be const.

nevi-me · 2020-09-13T05:36:01Z

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

wesm · 2020-09-13T21:25:35Z

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.

nevi-me · 2020-09-13T21:29:54Z

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.

Children should not be None/null, but should be an empty list

nevi-me · 2020-09-13T23:38:39Z

@wesm I found the problem, I was setting children as null instead of an empty list, so not a library issue. Thanks again

nevi-me · 2020-09-15T01:15:37Z

@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

paddyhoran

I didn't do an in-depth review but from a high level this looks good. Let's merge this and keep the momentum going. Great job @nevi-me

paddyhoran · 2020-09-15T13:48:29Z

@@ -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'];


Yea, I think this can be const.

maxburke · 2020-09-15T15:16:24Z

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

nevi-me · 2020-09-12T15:33:12Z

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.

nevi-me · 2020-09-12T15:34:50Z

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

github-actions · 2020-09-12T15:46:40Z

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

nevi-me · 2020-09-12T21:00:46Z

@@ -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'];


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.

Yea, I think this can be const.

nevi-me · 2020-09-13T05:36:01Z

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

wesm · 2020-09-13T21:25:35Z

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.

nevi-me · 2020-09-13T21:29:54Z

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.

Children should not be None/null, but should be an empty list

nevi-me · 2020-09-13T23:38:39Z

@wesm I found the problem, I was setting children as null instead of an empty list, so not a library issue. Thanks again

nevi-me · 2020-09-15T01:15:37Z

@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

paddyhoran

I didn't do an in-depth review but from a high level this looks good. Let's merge this and keep the momentum going. Great job @nevi-me

paddyhoran · 2020-09-15T13:48:29Z

@@ -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'];


Yea, I think this can be const.

maxburke · 2020-09-15T15:16:24Z

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

nevi-me · 2020-09-12T15:33:12Z

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.

nevi-me · 2020-09-12T15:34:50Z

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

github-actions · 2020-09-12T15:46:40Z

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

nevi-me · 2020-09-12T21:00:46Z

@@ -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'];


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.

Yea, I think this can be const.

nevi-me · 2020-09-13T05:36:01Z

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

wesm · 2020-09-13T21:25:35Z

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.

nevi-me · 2020-09-13T21:29:54Z

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.

Children should not be None/null, but should be an empty list

nevi-me · 2020-09-13T23:38:39Z

@wesm I found the problem, I was setting children as null instead of an empty list, so not a library issue. Thanks again

nevi-me · 2020-09-15T01:15:37Z

@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

paddyhoran

I didn't do an in-depth review but from a high level this looks good. Let's merge this and keep the momentum going. Great job @nevi-me

paddyhoran · 2020-09-15T13:48:29Z

@@ -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'];


Yea, I think this can be const.

maxburke · 2020-09-15T15:16:24Z

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

nevi-me · 2020-09-12T15:33:12Z

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.

nevi-me · 2020-09-12T15:34:50Z

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

github-actions · 2020-09-12T15:46:40Z

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

nevi-me · 2020-09-12T21:00:46Z

@@ -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'];


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.

Yea, I think this can be const.

nevi-me · 2020-09-13T05:36:01Z

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

wesm · 2020-09-13T21:25:35Z

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.

nevi-me · 2020-09-13T21:29:54Z

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.

Children should not be None/null, but should be an empty list

nevi-me · 2020-09-13T23:38:39Z

@wesm I found the problem, I was setting children as null instead of an empty list, so not a library issue. Thanks again

nevi-me · 2020-09-15T01:15:37Z

@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

paddyhoran

I didn't do an in-depth review but from a high level this looks good. Let's merge this and keep the momentum going. Great job @nevi-me

paddyhoran · 2020-09-15T13:48:29Z

@@ -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'];


Yea, I think this can be const.

maxburke · 2020-09-15T15:16:24Z

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

nevi-me · 2020-09-12T15:33:12Z

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.

nevi-me · 2020-09-12T15:34:50Z

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

github-actions · 2020-09-12T15:46:40Z

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

nevi-me · 2020-09-12T21:00:46Z

@@ -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'];


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.

Yea, I think this can be const.

nevi-me · 2020-09-13T05:36:01Z

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

wesm · 2020-09-13T21:25:35Z

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.

nevi-me · 2020-09-13T21:29:54Z

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.

Children should not be None/null, but should be an empty list

nevi-me · 2020-09-13T23:38:39Z

@wesm I found the problem, I was setting children as null instead of an empty list, so not a library issue. Thanks again

nevi-me · 2020-09-15T01:15:37Z

@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

paddyhoran

I didn't do an in-depth review but from a high level this looks good. Let's merge this and keep the momentum going. Great job @nevi-me

paddyhoran · 2020-09-15T13:48:29Z

@@ -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'];


Yea, I think this can be const.

maxburke · 2020-09-15T15:16:24Z

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

nevi-me · 2020-09-12T15:33:12Z

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.

nevi-me · 2020-09-12T15:34:50Z

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

github-actions · 2020-09-12T15:46:40Z

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

nevi-me · 2020-09-12T21:00:46Z

@@ -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'];


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.

Yea, I think this can be const.

nevi-me · 2020-09-13T05:36:01Z

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

wesm · 2020-09-13T21:25:35Z

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.

nevi-me · 2020-09-13T21:29:54Z

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.

Children should not be None/null, but should be an empty list

nevi-me · 2020-09-13T23:38:39Z

@wesm I found the problem, I was setting children as null instead of an empty list, so not a library issue. Thanks again

nevi-me · 2020-09-15T01:15:37Z

@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

paddyhoran

I didn't do an in-depth review but from a high level this looks good. Let's merge this and keep the momentum going. Great job @nevi-me

paddyhoran · 2020-09-15T13:48:29Z

@@ -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'];


Yea, I think this can be const.

maxburke · 2020-09-15T15:16:24Z

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

andygrove added the Component: Rust label Sep 12, 2020

ARROW-9848: [Rust] Support 0.15 IPC format
7eee08a

nevi-me force-pushed the ARROW-9848-on-clippy branch from ed4bb31 to 7eee08aCompare September 12, 2020 20:58

nevi-me requested review from andygrove, paddyhoran and sunchao September 12, 2020 20:59

nevi-me commented Sep 12, 2020
View reviewed changes

fix Null array ipc::Field
0a8a92d
Children should not be None/null, but should be an empty list

paddyhoran approved these changes Sep 15, 2020
View reviewed changes

andygrove approved these changes Sep 15, 2020
View reviewed changes

andygrove closed this in 75f804eSep 16, 2020

nevi-me deleted the ARROW-9848-on-clippy branch October 17, 2020 19:20

asfimport mentioned this pull request Sep 16, 2020
[Rust] Implement changes to ensure flatbuffer alignment #25886
Closed

andygrove added the Component: Rust label Sep 12, 2020

ARROW-9848: [Rust] Support 0.15 IPC format
7eee08a

nevi-me force-pushed the ARROW-9848-on-clippy branch from ed4bb31 to 7eee08aCompare September 12, 2020 20:58

nevi-me requested review from andygrove, paddyhoran and sunchao September 12, 2020 20:59

nevi-me commented Sep 12, 2020
View reviewed changes

fix Null array ipc::Field
0a8a92d
Children should not be None/null, but should be an empty list

paddyhoran approved these changes Sep 15, 2020
View reviewed changes

andygrove approved these changes Sep 15, 2020
View reviewed changes

andygrove closed this in 75f804eSep 16, 2020

nevi-me deleted the ARROW-9848-on-clippy branch October 17, 2020 19:20

asfimport mentioned this pull request Sep 16, 2020
[Rust] Implement changes to ensure flatbuffer alignment #25886
Closed

andygrove added the Component: Rust label Sep 12, 2020

ARROW-9848: [Rust] Support 0.15 IPC format
7eee08a

nevi-me force-pushed the ARROW-9848-on-clippy branch from ed4bb31 to 7eee08aCompare September 12, 2020 20:58

nevi-me requested review from andygrove, paddyhoran and sunchao September 12, 2020 20:59

nevi-me commented Sep 12, 2020
View reviewed changes

fix Null array ipc::Field
0a8a92d
Children should not be None/null, but should be an empty list

paddyhoran approved these changes Sep 15, 2020
View reviewed changes

andygrove approved these changes Sep 15, 2020
View reviewed changes

andygrove closed this in 75f804eSep 16, 2020

nevi-me deleted the ARROW-9848-on-clippy branch October 17, 2020 19:20

asfimport mentioned this pull request Sep 16, 2020
[Rust] Implement changes to ensure flatbuffer alignment #25886
Closed

andygrove added the Component: Rust label Sep 12, 2020

ARROW-9848: [Rust] Support 0.15 IPC format
7eee08a

nevi-me force-pushed the ARROW-9848-on-clippy branch from ed4bb31 to 7eee08aCompare September 12, 2020 20:58

nevi-me requested review from andygrove, paddyhoran and sunchao September 12, 2020 20:59

nevi-me commented Sep 12, 2020
View reviewed changes

fix Null array ipc::Field
0a8a92d
Children should not be None/null, but should be an empty list

paddyhoran approved these changes Sep 15, 2020
View reviewed changes

andygrove approved these changes Sep 15, 2020
View reviewed changes

andygrove closed this in 75f804eSep 16, 2020

nevi-me deleted the ARROW-9848-on-clippy branch October 17, 2020 19:20

asfimport mentioned this pull request Sep 16, 2020
[Rust] Implement changes to ensure flatbuffer alignment #25886
Closed

andygrove added the Component: Rust label Sep 12, 2020

ARROW-9848: [Rust] Support 0.15 IPC format
7eee08a

nevi-me force-pushed the ARROW-9848-on-clippy branch from ed4bb31 to 7eee08aCompare September 12, 2020 20:58

nevi-me requested review from andygrove, paddyhoran and sunchao September 12, 2020 20:59

nevi-me commented Sep 12, 2020
View reviewed changes

fix Null array ipc::Field
0a8a92d
Children should not be None/null, but should be an empty list

paddyhoran approved these changes Sep 15, 2020
View reviewed changes

andygrove approved these changes Sep 15, 2020
View reviewed changes

andygrove closed this in 75f804eSep 16, 2020

nevi-me deleted the ARROW-9848-on-clippy branch October 17, 2020 19:20

asfimport mentioned this pull request Sep 16, 2020
[Rust] Implement changes to ensure flatbuffer alignment #25886
Closed

andygrove added the Component: Rust label Sep 12, 2020

ARROW-9848: [Rust] Support 0.15 IPC format
7eee08a

nevi-me force-pushed the ARROW-9848-on-clippy branch from ed4bb31 to 7eee08aCompare September 12, 2020 20:58

nevi-me requested review from andygrove, paddyhoran and sunchao September 12, 2020 20:59

nevi-me commented Sep 12, 2020
View reviewed changes

fix Null array ipc::Field
0a8a92d
Children should not be None/null, but should be an empty list

paddyhoran approved these changes Sep 15, 2020
View reviewed changes

andygrove approved these changes Sep 15, 2020
View reviewed changes

andygrove closed this in 75f804eSep 16, 2020

nevi-me deleted the ARROW-9848-on-clippy branch October 17, 2020 19:20

asfimport mentioned this pull request Sep 16, 2020
[Rust] Implement changes to ensure flatbuffer alignment #25886
Closed

andygrove added the Component: Rust label Sep 12, 2020

ARROW-9848: [Rust] Support 0.15 IPC format
7eee08a

nevi-me force-pushed the ARROW-9848-on-clippy branch from ed4bb31 to 7eee08aCompare September 12, 2020 20:58

nevi-me requested review from andygrove, paddyhoran and sunchao September 12, 2020 20:59

nevi-me commented Sep 12, 2020
View reviewed changes

fix Null array ipc::Field
0a8a92d
Children should not be None/null, but should be an empty list

paddyhoran approved these changes Sep 15, 2020
View reviewed changes

andygrove approved these changes Sep 15, 2020
View reviewed changes

andygrove closed this in 75f804eSep 16, 2020

nevi-me deleted the ARROW-9848-on-clippy branch October 17, 2020 19:20

asfimport mentioned this pull request Sep 16, 2020
[Rust] Implement changes to ensure flatbuffer alignment #25886
Closed

andygrove added the Component: Rust label Sep 12, 2020

ARROW-9848: [Rust] Support 0.15 IPC format
7eee08a

nevi-me force-pushed the ARROW-9848-on-clippy branch from ed4bb31 to 7eee08aCompare September 12, 2020 20:58

nevi-me requested review from andygrove, paddyhoran and sunchao September 12, 2020 20:59

nevi-me commented Sep 12, 2020
View reviewed changes

fix Null array ipc::Field
0a8a92d
Children should not be None/null, but should be an empty list

paddyhoran approved these changes Sep 15, 2020
View reviewed changes

andygrove approved these changes Sep 15, 2020
View reviewed changes

andygrove closed this in 75f804eSep 16, 2020

nevi-me deleted the ARROW-9848-on-clippy branch October 17, 2020 19:20

asfimport mentioned this pull request Sep 16, 2020
[Rust] Implement changes to ensure flatbuffer alignment #25886
Closed

		@@ -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'];

Uh oh!

Conversation

nevi-me commented Sep 12, 2020

Uh oh!

nevi-me commented Sep 12, 2020

Uh oh!

github-actionsBot commented Sep 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nevi-me commented Sep 13, 2020

Uh oh!

wesm commented Sep 13, 2020

Uh oh!

nevi-me commented Sep 13, 2020

Uh oh!

nevi-me commented Sep 13, 2020

Uh oh!

nevi-me commented Sep 15, 2020

Uh oh!

paddyhoran left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxburke commented Sep 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

nevi-me commented Sep 12, 2020

Uh oh!

nevi-me commented Sep 12, 2020

Uh oh!

github-actionsBot commented Sep 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nevi-me commented Sep 13, 2020

Uh oh!

wesm commented Sep 13, 2020

Uh oh!

nevi-me commented Sep 13, 2020

Uh oh!

nevi-me commented Sep 13, 2020

Uh oh!

nevi-me commented Sep 15, 2020

Uh oh!

paddyhoran left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxburke commented Sep 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

nevi-me commented Sep 12, 2020

Uh oh!

nevi-me commented Sep 12, 2020

Uh oh!