chore(ci): Ensure integration workflow passes #643
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
We still have a note about flatbuffer alignment for We also have big-endian failures related to endian swapping. |
kou
pushed a commit
to apache/arrow
that referenced
this pull request
Oct 4, 2024
…compressed/dictionary-encoded files (#44298) ### Rationale for this change There are a few remaining failures when testing nanoarrow against itself: apache/arrow-nanoarrow#643 . Our IPC reader doesn't support dictionaries or compression, so we can't run those tests. ### What changes are included in this PR? Skips were added to the archery code that runs the tests. ### Are these changes tested? Yes (integration tests run on every commit) ### Are there any user-facing changes? No! * GitHub Issue: #44297 Authored-by: Dewey Dunnington <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
|
@bkietz Is there any chance you have the bandwidth to make sure I didn't miss anything here? |
bkietz
requested changes
Oct 4, 2024
src/nanoarrow/ipc/reader.c
Outdated
| @@ -230,6 +230,20 @@ static int ArrowIpcArrayStreamReaderNextHeader( | |||
| // propagated higher (e.g., if the stream is empty and there's no schema message) | |||
| ArrowErrorSet(&private_data->error, "No data available on stream"); | |||
| return ENODATA; | |||
| } else if (bytes_read == 4) { | |||
There was a problem hiding this comment.
I think this special case should specifically assert the metadata version we're currently reading. It's never valid for a V5 message to omit the continuation and a V4 message is invalid if it includes that.
And we can't write:
Suggested change
| } else if (bytes_read == 4) { | |
| // DON'T commit; broken! | |
| } else if (private_data->decoder.metadata_version == NANOARROW_IPC_METADATA_VERSION_V4 && bytes_read == 4) { |
because these peek etc functions are used before we've fully unpacked the schema to know what the metadata version is.
So I think what needs to happen is: the first time a decoder peeks a header and observes the lack of a continuation, we set ArrowIpcDecoderPrivate::prefix_length = 4. Later when we decode the metadata version, we can raise an error if the prefix length is not as expected. Also we should raise if the prefix length is inconsistent in any future message peeked by the decoder.
As a pleasant side effect, there will be one fewer argument for you to pass around :D
There was a problem hiding this comment.
In a fun turn of events, not all V4 metadata messages have a 4 byte prefix size (i.e., not in the 0.17.1 golden files). I think I included all the other checks you suggested! (And also opened an issue with instructions for how to skip those cases in the future should we decide they don't need to be supported): #648
|
I'm going to merge this to get the release candidate testing process started...feel free to leave comments and I can update the approach! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #641.
Unfortunately we just have to skip checking Rust compatibility due to apache/arrow-rs#5052 (e.g., apache/arrow-rs#6449 ).
This PR also ensures compatibility with big endian Arrow files and Arrow files from before the continuation token. Support for those had already been added in the decoder but hadn't made it to the stream reader yet.
Local check: