feat: append array views to arrays by rustyconover · Pull Request #930 · apache/arrow-nanoarrow

rustyconover · 2026-09-02T18:12:43Z

Summary

add ArrowArrayAppendArrayView() as a core array-building API
append primitive, nested, union, list-view, and run-end encoded values from an ArrowArrayView
preserve source slicing semantics and report unsupported sliced run-end encoded inputs
add direct native coverage for primitive, nested/sliced, union, and run-end encoded arrays

Context

This extracts the generic array-view appender from #928 as requested in review. It is independently useful and will be the prerequisite for a separate dictionary-delta decoding PR; #928 no longer contains the appender or decoder changes.

Validation

native CMake build passed
native CTest suite: 334/334 passed (4 optional codec tests skipped)

codecov-commenter · 2026-09-02T18:21:02Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.15%. Comparing base (6657945) to head (85ea0f7).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #930      +/-   ##
==========================================
- Coverage   79.49%   78.15%   -1.34%     
==========================================
  Files         106      106              
  Lines       16564    16859     +295     
  Branches     1986     1988       +2     
==========================================
+ Hits        13167    13176       +9     
- Misses       2164     2449     +285     
- Partials     1233     1234       +1

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

paleolimbot

Thank you!

Some things to take care of here but I think this is great functionality.

I can follow up with R/Python bindings, which are mostly useful because testing this stuff in C++ is a pain. I think this may also make a few of the R and Python constructors quite a bit simpler.

Add Arrow C++ conversion matrices plus sliced validity, overflow, dictionary, and sliced run-end tests. Preserve detailed errors produced by nested append helpers.

Avoid reading past short int8 or int32 input arrays when an append begins in a partially filled bitmap byte.

rustyconover · 2026-09-04T11:51:34Z

CI follow-up: b4f4adc fixes the shared failure behind clang-tidy and the four Valgrind jobs. ArrowBitmapAppendInt8Unsafe could read more values than supplied when a short append began in a partially filled byte; the same issue existed in the int32 variant. Both helpers now bound the partial-byte copy, with regression tests for short unaligned appends. Native, Arrow C++, AddressSanitizer, and local clang-tidy checks pass.

Ensure clang-tidy can prove that all values passed through the bitmap packing path are initialized.

rustyconover · 2026-09-04T13:13:12Z

clang-tidy follow-up: the Valgrind failures are resolved by b4f4adc, but clang-tidy 18 could not prove that ArrowBitsUnpackInt8 initialized every element later read from the scratch buffer. Commit 0603bc1 explicitly zero-initializes that buffer; this preserves behavior while guaranteeing analyzer-visible initialization.

paleolimbot

A few more things from this pass but I think it's close! On the next pass I'll send the AIs over it as well to look for coverage or correctness I'm not sure about.

Adds the int16 counterpart to ArrowResolveChunk32 and ArrowResolveChunk64. This follows up on the run-end encoded array review in #930, where int16 run ends currently require a linear scan. The helper uses the same binary-search semantics as the existing resolvers and includes boundary-parity tests. Validation: - 270/270 C/C++ tests passed - clang-format and git diff checks passed

rustyconover · 2026-09-06T13:00:05Z

@paleolimbot I think this is ready for your AI based review.

paleolimbot · 2026-09-07T01:24:32Z

Apologies for being slow here...I'll review when I'm back at work Tuesday!

paleolimbot

A few issues left:

Null storage appended to run-end encoded storage creates an invalid array. A run of nulls has to get added or you could also just reject this combination.
The conversion pathway is different in the nested case (e.g., list of decimal or struct containing decimals converts using the element-wise appender). I think the easiest way to do this is to update the signature of ArrowArrayAppendStorageFromArrayView() to accept an additional int64 offset and int64 length and recurse back using that (this is what the R and Python conversion functions do), but if there is another relatively non-verbose way to deal with that I'm happy to take a look.
I believe the compatibility check doesn't take into account struct names, and it probably should (appending xmin, xmax vs xmax, xmin is likely to be a problem should it be used for that).

When this merges I'll file a follow up for non-correctness issues that should get handled at some point, like the ability to handle non-storage conversions (e.g., appending timestamps with non-equal units), or speeding up conversions between integer widths / string on string appends.

rustyconover · 2026-09-09T14:48:00Z

Addressed the latest review in 2ae9cd6.

Null storage is now rejected when the destination uses run-end encoded storage, before any destination mutation.
Nested struct/list/fixed-size-list children now recurse through an explicit offset/length range helper, so nested fixed-width storage such as decimals follows the same validated bulk path as top-level storage.
Added regressions for null-to-REE rejection and decimal storage nested in both a struct and a list.
For struct field names: ArrowArrayView does not carry field names, and this API intentionally operates on storage rather than logical schema metadata. I documented that struct children are matched positionally and that callers must validate field names (as well as decimal precision/scale). The public API signature remains unchanged; offset/length are internal to the recursive helper.

Validation:

Native CMake/CTest: 280/280 passed.
Arrow C++-gated CMake/CTest: 320/320 passed.

paleolimbot

A few remaining issues...thank you for sticking with this! This is hard...you are basically implementing cast to/from all arrow types. Also very useful (we can use to implement explicit batch sizes on write, casting, and delta dictionaries!).

Codex turned up a few more things:

self-append causes use-after-free. I think this one can just be a comment in the docs...it is not normal/supported behaviour to view an array that is in append mode.
Two REE-related issues: REE destination-length overflow is undefined behavior and the Null→REE fix does not cover REE nested beneath a null parent. We've gone pretty far down the supporting REEs here so it's probably worth fixing, but you could also not support REEs here.

paleolimbot · 2026-09-09T18:57:50Z

+static ArrowErrorCode ArrowArrayAppendStorageFromArrayViewRange(
+    struct ArrowArray* dst, const struct ArrowArrayView* src, int64_t offset,
+    int64_t length, struct ArrowError* error) {
+  struct ArrowArrayView src_slice = *src;


This line is a little janky (it would be cleaner to pass the offset and length through the internal calls), but also OK as long as it is qualified. I can see how it's possibly less error prone to apply the slice once.

Suggested change

struct ArrowArrayView src_slice = *src;

// Note: src_slice must not be freed: this is used as a convenience to reduce the

// number of times the offset and length must be compose on top of src.

struct ArrowArrayView src_slice = *src;

Added the qualifying comment in 85ea0f7, including that the shallow slice must not be freed and why it is used.

paleolimbot · 2026-09-09T18:58:54Z

+    case NANOARROW_TYPE_INT16:
+    default: {
+      int64_t run = 0;
+      while (run + 1 < run_ends->length &&
+             ArrowArrayViewGetIntUnsafe(run_ends, run) <= logical_offset) {
+        run++;
+      }
+      return run;


Can you merge upstream/main and update this now that ArrowResolveChunk16() is merged?

Merged current upstream/main in 9690f71 and replaced the int16 linear scan with ArrowResolveChunk16() in 85ea0f7. The REE append test now exercises int16 run ends.

paleolimbot · 2026-09-09T19:06:41Z

+static int ArrowArrayCanAppendFixedWidthStorage(struct ArrowArray* dst,
+                                                const struct ArrowArrayView* src) {
+  struct ArrowArrayPrivateData* private_data =
+      (struct ArrowArrayPrivateData*)dst->private_data;
+  return private_data->storage_type == src->storage_type && dst->n_buffers == 2 &&
+         src->n_children == 0 &&
+         src->layout.buffer_type[1] == NANOARROW_BUFFER_TYPE_DATA &&
+         src->layout.element_size_bits[1] > 0 &&
+         src->layout.element_size_bits[1] % 8 == 0;
+}


I think the other case worth handling out of the gate here (or one level up) is a struct appended to a struct, which I believe is trivial (feel free to punt if it is not).

Added a bulk struct-to-struct path in 85ea0f7. It appends each child range and the parent validity in bulk; the regression test also covers a null struct slot with an REE child.

rustyconover · 2026-09-09T20:50:32Z

Addressed the remaining review points in 85ea0f7 (after merging current upstream/main in 9690f71):

documented that the source view must not reference destination-owned storage, since an append can reallocate it
qualified the shallow src_slice copy
switched int16 REE run lookup to ArrowResolveChunk16()
added bulk struct-to-struct appends, including valid child storage beneath null struct slots
reject other null appends before they can recursively create invalid REE child storage
guard REE destination length addition before any signed overflow or mutation

Added regression coverage for int16 REE lookup, struct/REE storage beneath a null parent, nested Null-to-REE rejection, and destination-length overflow.

Local validation: all 14 focused append tests pass; the full C/C++ suite passes 284/284 tests (3 expected thread-safety skips).

paleolimbot

Codex could only find one test coverage gap here (inline below). I'll open some follow on issues for R and Python integration (which should help get this code covered in a wider variety of scenarios). Thanks!

paleolimbot · 2026-09-10T17:04:42Z

+TEST(ArrayTest, ArrayAppendStorageFromArrayViewRunEndEncoded) {
+  struct ArrowError error;
+  struct ArrowSchema schema;
+  ArrowSchemaInit(&schema);
+  ASSERT_EQ(ArrowSchemaSetTypeRunEndEncoded(&schema, NANOARROW_TYPE_INT16), NANOARROW_OK);
+  ASSERT_EQ(ArrowSchemaSetType(schema.children[1], NANOARROW_TYPE_STRING), NANOARROW_OK);


Here the int32 REE and int64 REE branches aren't covered. I think a for() over NANOARROW_TYPE_INT16, NANOARROW_TYPE_INT32 and NANOARROW_TYPE_INT64 should cover those.

feat: append array views to arrays

58d145c

rustyconover mentioned this pull request Sep 2, 2026

feat(ipc): add dictionary batch write support #928

Open

paleolimbot reviewed Sep 2, 2026

View reviewed changes

rustyconover added 5 commits September 3, 2026 09:00

refactor: validate array view storage appends

bab29ad

perf: bulk append matching fixed-width storage

4974cd7

test: expand array view appender coverage

e56f47b

Add Arrow C++ conversion matrices plus sliced validity, overflow, dictionary, and sliced run-end tests. Preserve detailed errors produced by nested append helpers.

fix: allow fixed-to-variable storage appends

693718d

fix: bound partial bitmap appends

b4f4adc

Avoid reading past short int8 or int32 input arrays when an append begins in a partially filled bitmap byte.

fix: initialize validity append scratch buffer

0603bc1

Ensure clang-tidy can prove that all values passed through the bitmap packing path are initialized.

paleolimbot reviewed Sep 4, 2026

View reviewed changes

Comment thread src/nanoarrow/common/array.c Outdated

Comment thread src/nanoarrow/common/array.c Outdated

Comment thread src/nanoarrow/common/array.c

Comment thread src/nanoarrow/common/array.c

Comment thread src/nanoarrow/common/array.c Outdated

fix: narrow array view append support

dacf383

rustyconover mentioned this pull request Sep 5, 2026

feat: add ArrowResolveChunk16 #934

Merged

paleolimbot reviewed Sep 8, 2026

View reviewed changes

fix: handle nested array view storage appends

2ae9cd6

paleolimbot mentioned this pull request Sep 9, 2026

ArrowArrayAppendBytes has a narrowing cast when appending views with >INT32_MAX bytes #938

Open

paleolimbot reviewed Sep 9, 2026

View reviewed changes

rustyconover added 2 commits September 9, 2026 16:39

Merge remote-tracking branch 'origin/main' into feat/array-view-appender

9690f71

fix: handle array view append review edge cases

85ea0f7

paleolimbot reviewed Sep 10, 2026

View reviewed changes

Uh oh!

Conversation

rustyconover commented Sep 2, 2026

Summary

Context

Validation

Uh oh!

codecov-commenter commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

paleolimbot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rustyconover commented Sep 4, 2026

Uh oh!

rustyconover commented Sep 4, 2026

Uh oh!

paleolimbot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rustyconover commented Sep 6, 2026

Uh oh!

paleolimbot commented Sep 7, 2026

Uh oh!

paleolimbot left a comment

Choose a reason for hiding this comment

Uh oh!

rustyconover commented Sep 9, 2026

Uh oh!

paleolimbot left a comment

Choose a reason for hiding this comment

Uh oh!

paleolimbot Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

rustyconover Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

paleolimbot Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

rustyconover Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

paleolimbot Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

rustyconover Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

rustyconover commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paleolimbot left a comment

Choose a reason for hiding this comment

Uh oh!

paleolimbot Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Sep 2, 2026 •

edited

Loading

rustyconover commented Sep 9, 2026 •

edited

Loading