fix: Concatenate null and dictionary arrays

[CurtHagenlocher]

What's Changed

ArrayDataConcatenator (and so ArrowArrayConcatenator) couldn't handle two array types:

• Null arrays had no visitor case and threw NotImplementedException, at the top level or as a child (e.g. a null-typed struct field, which Parquet readers hit for pyarrow pa.null() columns). They now concatenate to a null array of the combined length.
• Dictionary arrays went through Visit(FixedWidthType) because DictionaryType derives from FixedWidthType. That concatenated the indices but dropped the dictionary, so ArrowArrayConcatenator threw Dictionary must not be null and ArrayDataConcatenator silently returned dictionary-typed data with no dictionary.

For dictionaries:

• If every non-empty input shares the same dictionary (the same ArrayData, or distinct ArrayData over the same memory, e.g. after Retain/SliceShared), the indices are concatenated and that dictionary is kept.
• Otherwise the dictionaries of the non-empty inputs are concatenated and each input's indices are shifted by the combined length of the dictionaries before it. Null slots get index 0 instead of a shifted, possibly out-of-range value. If the combined dictionary is too big for the index type, this throws OverflowException instead of wrapping around.
• Inputs with different index types or value types are rejected with ArgumentException.
• When different dictionaries are appended, the result is never marked ordered: nothing shows the appended entries are in order. When every input shares one dictionary, the result is ordered only if every input says so.

Dictionaries that are equal in content but stored separately take the concatenation path. The result is correct but not deduplicated. Unifying dictionaries could be added later if needed.

New tests cover null arrays, a struct with a null field, shared and different dictionaries (including nulls, slices and empty inputs), index overflow at the int8 boundary, the Ordered flag, and ArrayDataConcatenator keeping the dictionary.

Closes #446.

🤖 Generated with Claude Code

[Copilot]

Copilot review overview

🟡 Changes recommended

The ordered-dictionary correctness issue and exceptional-path memory leak must be addressed before approval.

Review effort: Lite
Findings: 1

Open (1)

• Preserve ordered dictionary semantics when combining dictionaries · New

What changed in this PR

Fixes concatenation of null and dictionary arrays, including nested null fields and dictionary index remapping.

Changes:

• Adds null-array concatenation.
• Preserves shared dictionaries and combines distinct dictionaries with overflow checks.
• Adds regression coverage for nulls, dictionaries, slicing, and overflow.

File	Summary	Findings
src/​Apache.Arrow/​Arrays/​ArrayDataConcatenator.cs	Implements null and dictionary concatenation.	Critical (2 votes): Combined dictionaries can incorrectly retain the first input’s Ordered flag. Moderate (1 vote): Validity buffers may leak when later validation or concatenation throws.
test/​Apache.Arrow.Tests/​ArrowArrayConcatenatorTests.cs	Tests null, dictionary, slicing, and overflow behavior.	Nit (1 vote): Add coverage for shared dictionary memory across distinct ArrayData instances and verify retained lifetime.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[CurtHagenlocher]

Good catch, fixed in e2d1b28. When different dictionaries are appended, the result is now never ordered: even if every input is ordered, the appended entries aren't necessarily in order, and nothing verifies that. When every input shares one dictionary, nothing is appended, so the result is ordered only if every input says so. That commit also moves the dictionary concatenation ahead of the buffer allocations and releases the buffers if building the indices throws, which covers the leak from the overview. It also adds a test for dictionaries held in separate ArrayData objects over the same memory that checks the result still works after the inputs are disposed.