paleolimbot commented on code in PR #979:
URL: https://github.com/apache/arrow-nanoarrow/pull/979#discussion_r4494048941
##########
src/nanoarrow/nanoarrow_ipc.h:
##########
@@ -1211,12 +1211,16 @@ NANOARROW_DLL ArrowErrorCode
ArrowIpcWriterWriteArrayView(struct ArrowIpcWriter*
/// assigned in the same order as when the schema was written, and nested
dictionaries
/// are written before the dictionaries that refer to them.
///
-/// Because in is borrowed, its dictionaries are not compared with those of
earlier
-/// batches (as ArrowIpcWriterWriteArrayStream() does to skip unchanged ones):
each
-/// dictionary is written in full before every RecordBatch, replacing the
previous
-/// one. In file mode, where a dictionary ID may only be written once, this
means only
-/// one batch with dictionaries can be written this way. in must not be NULL;
use
-/// ArrowIpcWriterWriteArrayView() to write EOS.
+/// A dictionary whose values are identical to the ones this function last
wrote for
+/// its ID is not written again. Because in is borrowed, the writer keeps a
copy of
+/// each dictionary it writes to compare with the next batch's (so the
comparison costs
+/// a copy of each dictionary per batch); values that can't be copied (e.g., a
dense
+/// union) are written every time. A changed dictionary is written in full,
replacing
Review Comment:
While I think it will be a while until somebody is up to the task of
dictionary encoding a dense union, remind me why it can't be copied?
##########
src/nanoarrow/ipc/writer.c:
##########
@@ -517,15 +578,255 @@ static ArrowErrorCode
ArrowIpcWriterWriteDictionariesForArray(
return NANOARROW_OK;
}
+// Initialize out as an array view with the same types as src
+static ArrowErrorCode ArrowIpcWriterArrayViewInitLike(struct ArrowArrayView*
out,
+ const struct
ArrowArrayView* src) {
+ ArrowArrayViewInitFromType(out, src->storage_type);
+ out->layout = src->layout;
+ NANOARROW_RETURN_NOT_OK(ArrowArrayViewAllocateChildren(out,
src->n_children));
+ for (int64_t i = 0; i < src->n_children; i++) {
+ NANOARROW_RETURN_NOT_OK(
+ ArrowIpcWriterArrayViewInitLike(out->children[i], src->children[i]));
+ }
+
+ if (src->dictionary != NULL) {
+ NANOARROW_RETURN_NOT_OK(ArrowArrayViewAllocateDictionary(out));
+ NANOARROW_RETURN_NOT_OK(
+ ArrowIpcWriterArrayViewInitLike(out->dictionary, src->dictionary));
+ }
+
+ return NANOARROW_OK;
+}
+
+// Producers need not initialize the padding bits of a bitmap's last byte.
Clear them
+// so that they don't make two copies of the same values compare unequal.
+static void ArrowIpcWriterClearBitmapPadding(struct ArrowArray* array,
+ const struct ArrowArrayView*
array_view) {
+ int64_t remainder = array->length % 8;
+ if (remainder != 0) {
+ uint8_t mask = (uint8_t)((1U << remainder) - 1U);
+ for (int i = 0; i < NANOARROW_MAX_FIXED_BUFFERS; i++) {
+ if (array_view->layout.element_size_bits[i] == 1) {
+ struct ArrowBuffer* buffer = ArrowArrayBuffer(array, i);
+ if (buffer->size_bytes > 0) {
+ buffer->data[buffer->size_bytes - 1] &= mask;
+ }
+ }
+ }
+ }
+
+ for (int64_t i = 0; i < array->n_children; i++) {
+ ArrowIpcWriterClearBitmapPadding(array->children[i],
array_view->children[i]);
+ }
+}
+
+// Copy the values of a dictionary into out, in a form that only depends on
the values:
+// zero offset, buffers sized to the values, and cleared bitmap padding. A
dictionary
+// nested in the values is not copied: it is written (and compared) separately.
+static ArrowErrorCode ArrowIpcWriterCopyDictionaryValues(
+ const struct ArrowArrayView* values_view, struct ArrowArray* out,
+ struct ArrowError* error) {
+ NANOARROW_RETURN_NOT_OK(ArrowArrayInitFromArrayView(out, values_view,
error));
+ ArrowErrorCode result = ArrowArrayStartAppending(out);
+ if (result == NANOARROW_OK) {
+ result = ArrowArrayReserve(out, values_view->length);
+ }
+ if (result == NANOARROW_OK) {
+ result = ArrowArrayAppendStorageFromArrayView(out, values_view, error);
+ }
+ if (result == NANOARROW_OK) {
+ result = ArrowArrayFinishBuildingDefault(out, error);
+ }
Review Comment:
Also elsewhere in this PR, but we typically handle this by nesting the
pieces that can fail (but whose failure requires cleanup) in an inner function,
so we only check the result and clean up once.
##########
src/nanoarrow/ipc/writer.c:
##########
@@ -517,15 +578,255 @@ static ArrowErrorCode
ArrowIpcWriterWriteDictionariesForArray(
return NANOARROW_OK;
}
+// Initialize out as an array view with the same types as src
+static ArrowErrorCode ArrowIpcWriterArrayViewInitLike(struct ArrowArrayView*
out,
+ const struct
ArrowArrayView* src) {
+ ArrowArrayViewInitFromType(out, src->storage_type);
+ out->layout = src->layout;
+ NANOARROW_RETURN_NOT_OK(ArrowArrayViewAllocateChildren(out,
src->n_children));
+ for (int64_t i = 0; i < src->n_children; i++) {
+ NANOARROW_RETURN_NOT_OK(
+ ArrowIpcWriterArrayViewInitLike(out->children[i], src->children[i]));
+ }
+
+ if (src->dictionary != NULL) {
+ NANOARROW_RETURN_NOT_OK(ArrowArrayViewAllocateDictionary(out));
+ NANOARROW_RETURN_NOT_OK(
+ ArrowIpcWriterArrayViewInitLike(out->dictionary, src->dictionary));
+ }
+
+ return NANOARROW_OK;
+}
+
+// Producers need not initialize the padding bits of a bitmap's last byte.
Clear them
+// so that they don't make two copies of the same values compare unequal.
+static void ArrowIpcWriterClearBitmapPadding(struct ArrowArray* array,
+ const struct ArrowArrayView*
array_view) {
+ int64_t remainder = array->length % 8;
+ if (remainder != 0) {
+ uint8_t mask = (uint8_t)((1U << remainder) - 1U);
+ for (int i = 0; i < NANOARROW_MAX_FIXED_BUFFERS; i++) {
+ if (array_view->layout.element_size_bits[i] == 1) {
+ struct ArrowBuffer* buffer = ArrowArrayBuffer(array, i);
+ if (buffer->size_bytes > 0) {
+ buffer->data[buffer->size_bytes - 1] &= mask;
+ }
+ }
+ }
+ }
+
+ for (int64_t i = 0; i < array->n_children; i++) {
+ ArrowIpcWriterClearBitmapPadding(array->children[i],
array_view->children[i]);
+ }
+}
+
+// Copy the values of a dictionary into out, in a form that only depends on
the values:
+// zero offset, buffers sized to the values, and cleared bitmap padding. A
dictionary
+// nested in the values is not copied: it is written (and compared) separately.
+static ArrowErrorCode ArrowIpcWriterCopyDictionaryValues(
+ const struct ArrowArrayView* values_view, struct ArrowArray* out,
+ struct ArrowError* error) {
+ NANOARROW_RETURN_NOT_OK(ArrowArrayInitFromArrayView(out, values_view,
error));
+ ArrowErrorCode result = ArrowArrayStartAppending(out);
+ if (result == NANOARROW_OK) {
+ result = ArrowArrayReserve(out, values_view->length);
+ }
+ if (result == NANOARROW_OK) {
+ result = ArrowArrayAppendStorageFromArrayView(out, values_view, error);
+ }
+ if (result == NANOARROW_OK) {
+ result = ArrowArrayFinishBuildingDefault(out, error);
+ }
+ if (result != NANOARROW_OK) {
+ ArrowArrayRelease(out);
+ return result;
+ }
+
+ ArrowIpcWriterClearBitmapPadding(out, values_view);
+ return NANOARROW_OK;
+}
+
+// Compare two copies made by ArrowIpcWriterCopyDictionaryValues() from views
of
+// the same type as values_view
+static ArrowErrorCode ArrowIpcWriterDictionaryValuesEqual(
+ const struct ArrowArrayView* values_view, const struct ArrowArray* lhs,
+ const struct ArrowArray* rhs, int* out, struct ArrowError* error) {
+ struct ArrowArrayView lhs_view;
+ struct ArrowArrayView rhs_view;
+ ArrowArrayViewInitFromType(&lhs_view, NANOARROW_TYPE_UNINITIALIZED);
+ ArrowArrayViewInitFromType(&rhs_view, NANOARROW_TYPE_UNINITIALIZED);
+
+ ArrowErrorCode result = ArrowIpcWriterArrayViewInitLike(&lhs_view,
values_view);
+ if (result == NANOARROW_OK) {
+ result = ArrowIpcWriterArrayViewInitLike(&rhs_view, values_view);
+ }
+ if (result == NANOARROW_OK) {
+ result = ArrowArrayViewSetArray(&lhs_view, lhs, error);
+ }
+ if (result == NANOARROW_OK) {
+ result = ArrowArrayViewSetArray(&rhs_view, rhs, error);
+ }
+ if (result == NANOARROW_OK) {
+ result = ArrowArrayViewCompare(&lhs_view, &rhs_view,
NANOARROW_COMPARE_IDENTICAL, out,
+ NULL);
+ }
Review Comment:
Here as well
##########
src/nanoarrow/ipc/writer.c:
##########
@@ -517,15 +578,255 @@ static ArrowErrorCode
ArrowIpcWriterWriteDictionariesForArray(
return NANOARROW_OK;
}
+// Initialize out as an array view with the same types as src
+static ArrowErrorCode ArrowIpcWriterArrayViewInitLike(struct ArrowArrayView*
out,
+ const struct
ArrowArrayView* src) {
Review Comment:
If we don't have a `ArrowArrayViewInitFromArrayView()`, I think this would
be a good name for it and a better place for it would be nanoarrow.h / array.c
so that future internals reuse it. I thought we had an internal one already but
that may have been a different combination.
##########
src/nanoarrow/ipc/writer.c:
##########
@@ -517,15 +578,255 @@ static ArrowErrorCode
ArrowIpcWriterWriteDictionariesForArray(
return NANOARROW_OK;
}
+// Initialize out as an array view with the same types as src
+static ArrowErrorCode ArrowIpcWriterArrayViewInitLike(struct ArrowArrayView*
out,
+ const struct
ArrowArrayView* src) {
+ ArrowArrayViewInitFromType(out, src->storage_type);
+ out->layout = src->layout;
+ NANOARROW_RETURN_NOT_OK(ArrowArrayViewAllocateChildren(out,
src->n_children));
+ for (int64_t i = 0; i < src->n_children; i++) {
+ NANOARROW_RETURN_NOT_OK(
+ ArrowIpcWriterArrayViewInitLike(out->children[i], src->children[i]));
+ }
+
+ if (src->dictionary != NULL) {
+ NANOARROW_RETURN_NOT_OK(ArrowArrayViewAllocateDictionary(out));
+ NANOARROW_RETURN_NOT_OK(
+ ArrowIpcWriterArrayViewInitLike(out->dictionary, src->dictionary));
+ }
+
+ return NANOARROW_OK;
+}
+
+// Producers need not initialize the padding bits of a bitmap's last byte.
Clear them
+// so that they don't make two copies of the same values compare unequal.
+static void ArrowIpcWriterClearBitmapPadding(struct ArrowArray* array,
+ const struct ArrowArrayView*
array_view) {
+ int64_t remainder = array->length % 8;
+ if (remainder != 0) {
+ uint8_t mask = (uint8_t)((1U << remainder) - 1U);
+ for (int i = 0; i < NANOARROW_MAX_FIXED_BUFFERS; i++) {
+ if (array_view->layout.element_size_bits[i] == 1) {
+ struct ArrowBuffer* buffer = ArrowArrayBuffer(array, i);
+ if (buffer->size_bytes > 0) {
+ buffer->data[buffer->size_bytes - 1] &= mask;
+ }
+ }
+ }
+ }
+
+ for (int64_t i = 0; i < array->n_children; i++) {
+ ArrowIpcWriterClearBitmapPadding(array->children[i],
array_view->children[i]);
+ }
+}
+
+// Copy the values of a dictionary into out, in a form that only depends on
the values:
+// zero offset, buffers sized to the values, and cleared bitmap padding. A
dictionary
+// nested in the values is not copied: it is written (and compared) separately.
+static ArrowErrorCode ArrowIpcWriterCopyDictionaryValues(
Review Comment:
You can have claude double check this, but Codex flagged for me that null
storage slots aren't necessarily equal but may be considered equal by
`ArrowArrayViewCompare()`. I believe we can either fix that in
ArrowArrayViewCompare, fix that in AppendStorage (by writing canonical null
slots), or flag that as a limitation of this function in a comment. In this
case, I think that would be rare and wouldn't result in a correctness issue
(just an extra emitted dictionary).
--
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]
