-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Relax comparison strictness such that integration tests pass #399
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #399 +/- ##
==========================================
+ Coverage 88.76% 88.80% +0.03%
==========================================
Files 82 82
Lines 14633 14685 +52
==========================================
+ Hits 12989 13041 +52
Misses 1644 1644 ☔ View full report in Codecov by Sentry. |
f5d8c46
to
fc2afa2
Compare
627fda1
to
ab969ce
Compare
ArrowStringView key, value; | ||
while (reader.remaining_keys > 0) { | ||
NANOARROW_RETURN_NOT_OK(ArrowMetadataReaderRead(&reader, &key, &value)); | ||
out->insert({std::string(key.data, key.size_bytes), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be sure, can we safely rule out the possibility of duplicate keys by this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! I will workshop this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok! I now check for duplicate keys, although technically the comparison will just fail if this happens. The real solution is probably to fix the Java implementation to stop reordering metadata and add test cases for this in the archery data generator.
…e Arrays (#41054) ### Rationale for this change When implementing integration tests for nanoarrow, it was observed that C# never released arrays where `array->buffers[i]` was `NULL` (including any buffers of any recursive child arrays). This is allowed ( https://arrow.apache.org/docs/format/CDataInterface.html#c.ArrowArray.buffers ); however, every other implementation appears to allocate even for length zero buffers (including nanoarrow after apache/arrow-nanoarrow#399 ). ### What changes are included in this PR? `AddMemory()` is replaced with `ArrowBuffer.Empty` if the length of the imported buffer would have been 0 bytes. For other buffers (or anywhere I saw dereferencing a buffer pointer), I added a `Debug.Assert` just to be sure. ### Are these changes tested? I'm not sure what the best way to test them is! They won't be tested in the nanoarrow integration tests since at the point that they run, nanoarrow will no longer export arrays that would trigger this. ### Are there any user-facing changes? No * GitHub Issue: #40898 Authored-by: Dewey Dunnington <dewey@fishandwhistle.net> Signed-off-by: Curt Hagenlocher <curt@hagenlocher.org>
…terface Arrays (apache#41054) ### Rationale for this change When implementing integration tests for nanoarrow, it was observed that C# never released arrays where `array->buffers[i]` was `NULL` (including any buffers of any recursive child arrays). This is allowed ( https://arrow.apache.org/docs/format/CDataInterface.html#c.ArrowArray.buffers ); however, every other implementation appears to allocate even for length zero buffers (including nanoarrow after apache/arrow-nanoarrow#399 ). ### What changes are included in this PR? `AddMemory()` is replaced with `ArrowBuffer.Empty` if the length of the imported buffer would have been 0 bytes. For other buffers (or anywhere I saw dereferencing a buffer pointer), I added a `Debug.Assert` just to be sure. ### Are these changes tested? I'm not sure what the best way to test them is! They won't be tested in the nanoarrow integration tests since at the point that they run, nanoarrow will no longer export arrays that would trigger this. ### Are there any user-facing changes? No * GitHub Issue: apache#40898 Authored-by: Dewey Dunnington <dewey@fishandwhistle.net> Signed-off-by: Curt Hagenlocher <curt@hagenlocher.org>
…terface Arrays (apache#41054) ### Rationale for this change When implementing integration tests for nanoarrow, it was observed that C# never released arrays where `array->buffers[i]` was `NULL` (including any buffers of any recursive child arrays). This is allowed ( https://arrow.apache.org/docs/format/CDataInterface.html#c.ArrowArray.buffers ); however, every other implementation appears to allocate even for length zero buffers (including nanoarrow after apache/arrow-nanoarrow#399 ). ### What changes are included in this PR? `AddMemory()` is replaced with `ArrowBuffer.Empty` if the length of the imported buffer would have been 0 bytes. For other buffers (or anywhere I saw dereferencing a buffer pointer), I added a `Debug.Assert` just to be sure. ### Are these changes tested? I'm not sure what the best way to test them is! They won't be tested in the nanoarrow integration tests since at the point that they run, nanoarrow will no longer export arrays that would trigger this. ### Are there any user-facing changes? No * GitHub Issue: apache#40898 Authored-by: Dewey Dunnington <dewey@fishandwhistle.net> Signed-off-by: Curt Hagenlocher <curt@hagenlocher.org>
These changes are the changes required such that apache/arrow#39302 results in passing integration tests for nanoarrow. The changes are mostly related to comparison: