paleolimbot commented on code in PR #928:
URL: https://github.com/apache/arrow-nanoarrow/pull/928#discussion_r4254097887
##########
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:
I think the only exception here is for StringView or BinaryView, where its
final buffer is a buffer of sizes that I think is typically generated on each
export and may not be the same.
(IPC for nanoarrow doesn't currently support this but someday it will and is
probably worth fixing now)
##########
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:
Codex flagged for me here that `ENOTSUP` would be more consistent with the
other things that can't be done in IPC file writing, although I don't
personally mind either one
--
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]
