rustyconover commented on code in PR #928:
URL: https://github.com/apache/arrow-nanoarrow/pull/928#discussion_r4296642635
##########
src/nanoarrow/ipc/writer.c:
##########
@@ -378,39 +400,79 @@ ArrowErrorCode ArrowIpcWriterWriteDictionaryBatch(
return NANOARROW_OK;
}
+// Check whether two arrays are guaranteed to contain identical values because
they
+// have the same shape and point to the same buffers (e.g., a dictionary
shared by
+// several arrays in a stream). This is only meaningful while both arrays are
alive:
+// once an array is released, the addresses of its buffers may be reused for
+// different values.
+static int ArrowIpcWriterArraysShareBuffers(const struct ArrowArray* lhs,
+ const struct ArrowArray* rhs) {
+ if (lhs->length != rhs->length || lhs->offset != rhs->offset ||
+ lhs->null_count != rhs->null_count || lhs->n_buffers != rhs->n_buffers ||
+ lhs->n_children != rhs->n_children ||
+ (lhs->dictionary == NULL) != (rhs->dictionary == NULL)) {
+ return 0;
+ }
+
+ for (int64_t i = 0; i < lhs->n_buffers; i++) {
+ if (lhs->buffers[i] != rhs->buffers[i]) {
Review Comment:
Good catch. `ArrowIpcWriterArraysShareBuffers()` now gets the
`ArrowArrayView` alongside the arrays (for the storage types) and skips the
last buffer of a BinaryView/StringView array. It still compares the views and
variadic data buffers, which hold the values. I noted this in the
`WriteArrayStream()` docs too. I couldn't add a test that reaches it, because
the encoder can't write view types yet.
##########
src/nanoarrow/ipc/writer.c:
##########
@@ -335,15 +347,23 @@ ArrowErrorCode ArrowIpcWriterWriteDictionaryBatch(
struct ArrowIpcWriterPrivate* private =
(struct ArrowIpcWriterPrivate*)writer->private_data;
- // This check is intentionally minimal: we're allowed to write one dictionary
- // batch per ID in a file but we would need to add bookkeeping to keep track
- // of written IDs (and usefully a fingerprint or reference to the dictionary
- // so we can check if we need to emit it again).
- if (private->writing_file &&
- (is_delta || private->footer.dictionary_blocks.size_bytes != 0)) {
- ArrowErrorSet(error,
- "IPC file writing supports exactly one non-delta dictionary
batch");
- return ENOTSUP;
+ if (private->writing_file) {
+ if (is_delta) {
+ ArrowErrorSet(error, "IPC file writing does not support delta dictionary
batches");
+ return ENOTSUP;
+ }
+
+ const int64_t* written_ids = (const
int64_t*)private->file_dictionary_ids.data;
+ int64_t n_written_ids =
+ private->file_dictionary_ids.size_bytes / (int64_t)sizeof(int64_t);
+ for (int64_t i = 0; i < n_written_ids; i++) {
+ if (written_ids[i] == dictionary_id) {
+ ArrowErrorSet(
+ error, "Arrow IPC files do not support replacement of dictionary
ID %" PRId64,
+ dictionary_id);
+ return EINVAL;
Review Comment:
Switched to `ENOTSUP`, to match the delta case right above it, and updated
the docs for `WriteDictionaryBatch()` and `WriteArrayStream()` and the two
tests.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
