feat: Add support for run-end encoded array #507

cocoa-xu · 2024-06-04T21:10:00Z

Hi this PR tries to add support for run-end encoded array based on the arrow spec here, https://arrow.apache.org/docs/format/Columnar.html#run-end-encoded-layout.

paleolimbot

This is very clean and I look forward to giving it a closer look tomorrow!

@felipecrv if you have the bandwidth, would you mind taking a look through this for the details of the run-end encoding?

src/nanoarrow/nanoarrow_types.h

felipecrv

Initial review with some quick thoughts.

src/nanoarrow/array.c

cocoa-xu · 2024-06-05T10:39:13Z

Hi @felipecrv, many thanks for the code review and these examples! I've adapted some code from the code snippets in the review, and it should handle the validation properly now. :)

paleolimbot

Thank you again (and thanks to Felipe for doing a first pass on the details)! I have a few comments here for discussion 🙂

src/nanoarrow/array.c

src/nanoarrow/array_test.cc

paleolimbot · 2024-06-05T13:31:01Z

src/nanoarrow/array_inline.h

+      array->length = 0;
+      struct ArrowBuffer* data_buffer;
+      data_buffer = ArrowArrayBuffer(array->children[0], 1);
+      for (int64_t i = 0; i < array->children[0]->length; i++) {


I am not sure that you need a loop here? (i.e., is it possible to just extract the last run end?)

I wonder if this would be a better fit for ArrowArrayFinishBuilding()? Or maybe we need FinishAppending()? I am not sure I would have thought of FinishElement to do these checks/updates since they really only need to happen once per array.

Maybe ArrowArrayFinishRunEndEncoded might be better as it's more suggestive from its name and less confusion?

Perhaps, but it is also very similar to the piece of code we have that updates the null_count based on the validity buffer, which I think happens in ArrowArrayFinishBuilding() (although that "feature" has caused me some personal confusion because I sometimes forget that it happens and wonder why the null_count isn't what I set it to be).

The most backward compatible thing to do would be to not do any updating of lengths (i.e., force the caller to set the length of the parent run-end-encoded array). This would not necessarily be difficult to do because they would have to be keeping track of that length to append to the run-ends child (so they would have to keep a counter somewhere anyway). Perhaps for this PR the helper could be omitted and we can learn from the experience of implementing this elsewhere what the best intervention would be?

Perhaps for this PR the helper could be omitted and we can learn from the experience of implementing this elsewhere what the best intervention would be?

No problem, I've removed it :)

src/nanoarrow/array_inline.h

src/nanoarrow/nanoarrow.h

codecov-commenter · 2024-06-05T16:15:45Z

Codecov Report

Attention: Patch coverage is 69.56522% with 28 lines in your changes missing coverage. Please review.

Project coverage is 88.55%. Comparing base (8894ebf) to head (0ce34e5).
Report is 10 commits behind head on main.

Files	Patch %	Lines
src/nanoarrow/array.c	55.55%	28 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #507      +/-   ##
==========================================
- Coverage   89.21%   88.55%   -0.66%     
==========================================
  Files          90       90              
  Lines       16294    16541     +247     
==========================================
+ Hits        14536    14648     +112     
- Misses       1758     1893     +135

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

cocoa-xu · 2024-06-05T17:18:39Z

Just added more tests to cover more lines and the memory issue in the schema test should be fixed.

paleolimbot

Just a few nits, but this looks good from my end!

@felipecrv are there any more REE-specific details relevant to this PR (or follow-ups that we need to open issues for and tackle before the next release?)

src/nanoarrow/nanoarrow.h

src/nanoarrow/array.c

felipecrv · 2024-06-07T18:30:49Z

Just a few nits, but this looks good from my end!

@felipecrv are there any more REE-specific details relevant to this PR (or follow-ups that we need to open issues for and tackle before the next release?)

Not that I'm aware right now. I recommend that you and @cocoa-xu take a look at the C++ code around REEs to see all the traps. I don't remember everything that I did, but I left many comments in the C++ implementation.

https://github.com/apache/arrow/blob/main/cpp/src/arrow/util/ree_util.h
and
https://github.com/apache/arrow/blob/main/cpp/src/arrow/util/ree_util.cc
are good sources to look at.

Tests for #507 and #501 and/or #503 either used C++17 or features from Arrow C++ > 12. Our test suite still supports these (although perhaps parts of this support should be dropped soon). On Windows, formatting with `%lu` was doing some unexpected formatting. We could do a better job formatting 64-bit integers in error messages (e.g., using `PRId64` and the requisite defines to ensure it works on mingw); however, we probably won't ever be able to support properly formatting an unsigned 64-bit integer on every platform we support. I changed the error message (and its test) slightly to reflect that.

WillAyd · 2024-06-11T04:07:00Z

I think this PR introduced a regression that valgrind is complaining about. You can see it started back in the weekly Meson build:

https://github.com/apache/arrow-nanoarrow/actions/runs/9432588616

A more detailed error description can be found here:

https://github.com/apache/arrow-nanoarrow/actions/runs/9457996609/job/26052769002?pr=483#step:7:319

Apparently Valgrind does not like something that ultimately hits this codeblock from the RunEndEncoded tests:

arrow-nanoarrow/src/nanoarrow/array.c

Line 1247 in f443e46

if (array_view->storage_type == NANOARROW_TYPE_RUN_END_ENCODED) {

WillAyd · 2024-06-11T04:08:40Z

src/nanoarrow/array.c

+      }
+      last_run_end = run_end;
+    }
+    last_run_end = ArrowArrayViewGetIntUnsafe(run_ends_view, run_ends_view->length - 1);


last_run_end = ArrowArrayViewGetIntUnsafe(run_ends_view, run_ends_view->length - 1);

Is there any chance of run_ends_view->length being 0 here? If so I think this might invoke UB

Good catch! I'll send a PR

feat: Add support for run-end encoded array

b7b280e

paleolimbot reviewed Jun 5, 2024

View reviewed changes

src/nanoarrow/nanoarrow_types.h Outdated Show resolved Hide resolved

cleaned unused code

65d9185

felipecrv requested changes Jun 5, 2024

View reviewed changes

src/nanoarrow/array.c Outdated Show resolved Hide resolved

src/nanoarrow/array.c Outdated Show resolved Hide resolved

src/nanoarrow/array.c Outdated Show resolved Hide resolved

src/nanoarrow/array.c Outdated Show resolved Hide resolved

addressed issues in code review

89a0f1d

cocoa-xu force-pushed the cx-run-end-encoded branch from 037d31a to 89a0f1d Compare June 5, 2024 10:41

paleolimbot reviewed Jun 5, 2024

View reviewed changes

cocoa-xu added 4 commits June 5, 2024 15:48

addressed issues in code review

7d52451

fix schema test fr run-end encoded array

d6f0a05

the length of a run-end encoded array should be maunally updated

7d39533

fix schema test

0ce34e5

cocoa-xu added 3 commits June 5, 2024 17:23

fix schema test

a50c84d

add more tests in ArrayTestAppendToRunEndEncodedArray

8986399

add more tests for run-end encoded array

7953380

paleolimbot approved these changes Jun 5, 2024

View reviewed changes

src/nanoarrow/nanoarrow.h Outdated Show resolved Hide resolved

src/nanoarrow/nanoarrow.h Outdated Show resolved Hide resolved

src/nanoarrow/array.c Outdated Show resolved Hide resolved

addressed issues in code review

e625704

felipecrv approved these changes Jun 7, 2024

View reviewed changes

paleolimbot mentioned this pull request Jun 7, 2024

Finalize run-end-encoding support #510

Open

paleolimbot merged commit c4d20a0 into apache:main Jun 7, 2024
32 checks passed

cocoa-xu deleted the cx-run-end-encoded branch June 7, 2024 19:59

paleolimbot mentioned this pull request Jun 10, 2024

chore(ci): Ensure tests pass for Arrow C++ <12 and on C++11 #514

Merged

WillAyd reviewed Jun 11, 2024

View reviewed changes

WillAyd mentioned this pull request Jun 11, 2024

refactor: Use inttypes.h macros instead of casts to print fixed-width integers #520

Merged

paleolimbot added this to the nanoarrow 0.6.0 milestone Sep 17, 2024

WillAyd mentioned this pull request Jan 22, 2025

MINOR: [Docs] Update implementation status for nanoarrow apache/arrow#45333

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for run-end encoded array #507

feat: Add support for run-end encoded array #507

cocoa-xu commented Jun 4, 2024

paleolimbot left a comment

felipecrv left a comment

cocoa-xu commented Jun 5, 2024

paleolimbot left a comment

paleolimbot Jun 5, 2024

cocoa-xu Jun 5, 2024

paleolimbot Jun 5, 2024

cocoa-xu Jun 5, 2024

codecov-commenter commented Jun 5, 2024

cocoa-xu commented Jun 5, 2024

paleolimbot left a comment

felipecrv commented Jun 7, 2024

WillAyd commented Jun 11, 2024

WillAyd Jun 11, 2024

cocoa-xu Jun 11, 2024

feat: Add support for run-end encoded array #507

feat: Add support for run-end encoded array #507

Conversation

cocoa-xu commented Jun 4, 2024

paleolimbot left a comment

Choose a reason for hiding this comment

felipecrv left a comment

Choose a reason for hiding this comment

cocoa-xu commented Jun 5, 2024

paleolimbot left a comment

Choose a reason for hiding this comment

paleolimbot Jun 5, 2024

Choose a reason for hiding this comment

cocoa-xu Jun 5, 2024

Choose a reason for hiding this comment

paleolimbot Jun 5, 2024

Choose a reason for hiding this comment

cocoa-xu Jun 5, 2024

Choose a reason for hiding this comment

codecov-commenter commented Jun 5, 2024

Codecov Report

cocoa-xu commented Jun 5, 2024

paleolimbot left a comment

Choose a reason for hiding this comment

felipecrv commented Jun 7, 2024

WillAyd commented Jun 11, 2024

WillAyd Jun 11, 2024

Choose a reason for hiding this comment

cocoa-xu Jun 11, 2024

Choose a reason for hiding this comment