GH-32104: [C++] Add support for Run-End encoded data to Arrow #33641

felipecrv · 2023-01-12T20:26:09Z

This PR gathers work from multiple PRs that can be closed after this one is merged:

And the issues associated with those PRs can also be closed:

Fixes [C++] Handling of array-only types using VisitTypeInline #20350
Add RunEndEncodedScalarType
Fixes [C++] Add type ID, Type and Array classes for RLE #32543
Fixes [C++] Utility functions for working with RLE #32544
Fixes [C++] Array Builder for RLE Arrays #32688
Fixes [C++] Validation for RLE arrays #32731
Fixes [C++] Compare RLE Arrays #32772
Fixes [C++] Concatenate implementation for RLE #32774

Closes: [Format][C++][Go] Adding Run-Length encoding to Arrow #32104

github-actions · 2023-01-12T20:26:36Z

Closes: [Format][C++][Go] Adding Run-Length encoding to Arrow #32104

github-actions · 2023-01-12T20:26:39Z

⚠️ GitHub issue #32104 has been automatically assigned in GitHub to PR creator.

github-actions · 2023-01-17T17:27:44Z

⚠️ GitHub issue #32104 has been automatically assigned in GitHub to PR creator.

pitrou

This is a first pass on basic facilities. I didn't look at kernels and REE utils yet.

cpp/src/arrow/CMakeLists.txt

cpp/src/arrow/array.h

cpp/src/arrow/type_test.cc

pitrou · 2023-01-18T09:58:14Z

cpp/src/arrow/type_traits.h

@@ -1177,6 +1197,7 @@ constexpr bool is_nested(Type::type type_id) {
    case Type::STRUCT:
    case Type::SPARSE_UNION:
    case Type::DENSE_UNION:
+    case Type::RUN_END_ENCODED:


I'm not sure this is right? These types are semantically (logically) nested. Run-end encoded arrays are only physically nested. I don't know which choice makes the most sense and/or requires the least special-casing.

@lidavidm @bkietz What do you think?

From a quick glance, I think this is mostly used as 'physically nested' in the codebase, so this is OK. I don't think we have the right abstractions to differentiate between physical/logical and deal with encodings, e.g. the way we handle dictionaries is often to just decode them, and kernel implementations are very much often lost in the weeds of encoding details.

cpp/src/arrow/type.h

cpp/src/arrow/array/builder_encoded.cc

cpp/src/arrow/array/concatenate.cc

cpp/src/parquet/arrow/path_internal.cc

cpp/src/arrow/visitor_generate.h

pitrou

Second part of code review: it doesn't seem clear what the offset and length of a run-end encoded array mean. Once this is settled and applied consistently, I'll be able to do a third review pass.

cpp/src/arrow/compute/kernels/vector_run_end_encode_test.cc

cpp/src/arrow/array/validate.cc

cpp/src/arrow/array/array_encoded.cc

cpp/src/arrow/array/array_encoded.h

cpp/src/arrow/array/validate.cc

cpp/src/arrow/CMakeLists.txt

cpp/src/arrow/array.h

github-actions · 2023-02-01T22:31:00Z

⚠️ GitHub issue #32104 has been automatically assigned in GitHub to PR creator.

felipecrv · 2023-02-03T14:48:06Z

@lidavidm @wjones127 @westonpace @zeroshade @bkietz

Before this being fully ready for review, I want to:

add the Scalar type and remove the VisitTypeInline and
I want to rewrite the comparator code
more tests

but feel free to skim the code and give higher level architectural feedback before a more detailed review

Lead-authored-by: Tobias Zagorni <tobias@zagorni.eu> Co-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

Since jemalloc.h does # define malloc je_malloc when JEMALLOC_MANGLE is defined, we can get this error in CI during an unity build /arrow/cpp/src/arrow/vendored/ProducerConsumerQueue.h: In constructor 'arrow_vendored::folly::ProducerConsumerQueue<T>::ProducerConsumerQueue(uint32_t)': /arrow/cpp/src/arrow/vendored/ProducerConsumerQueue.h:82:39: error: 'je_arrow_malloc' is not a member of 'std'; did you mean 'je_arrow_malloc'? 82 | records_(static_cast<T*>(std::malloc(sizeof(T) * size))), | ^~~~~~ jemalloc_ep-prefix/src/jemalloc_ep/dist/include/jemalloc/jemalloc.h:254:32: note: 'je_arrow_malloc' declared here 254 | void JEMALLOC_SYS_NOTHROW *je_malloc(size_t size) | ^~~~~~~~~ /arrow/cpp/src/arrow/vendored/ProducerConsumerQueue.h: In destructor 'arrow_vendored::folly::ProducerConsumerQueue<T>::~ProducerConsumerQueue()': /arrow/cpp/src/arrow/vendored/ProducerConsumerQueue.h:106:10: error: 'je_arrow_free' is not a member of 'std'; did you mean 'je_arrow_free'? 106 | std::free(records_); | ^~~~ jemalloc_ep-prefix/src/jemalloc_ep/dist/include/jemalloc/jemalloc.h:269:43: note: 'je_arrow_free' declared here 269 | JEMALLOC_EXPORT void JEMALLOC_SYS_NOTHROW je_free(void *ptr) | ^~~~~~~

felipecrv · 2023-02-16T11:42:56Z

@zeroshade this is now passing all the builds.

zeroshade

LGTM

cpp/src/arrow/type.h

cpp/src/arrow/testing/json_internal.cc

cpp/src/arrow/array/array_test.cc

cpp/src/arrow/array/array_base.cc

cpp/src/arrow/util/ree_util.h

cpp/src/arrow/util/ree_util_test.cc

cpp/src/arrow/scalar.cc

cpp/src/arrow/array/concatenate.cc

cpp/src/arrow/array/builder_run_end.cc

lidavidm

Thanks!

felipecrv · 2023-02-17T15:01:49Z

@lidavidm should I rebase and force push so this CI issue goes away or this can be merged as is?

lidavidm · 2023-02-17T15:17:49Z

I kicked the builds, let's see.

…pache#33641) This PR gathers work from multiple PRs that can be closed after this one is merged: - Closes apache#13752 - Closes apache#13754 - Closes apache#13842 - Closes apache#13882 - Closes apache#13916 - Closes apache#14063 - Closes apache#13970 And the issues associated with those PRs can also be closed: - Fixes apache#20350 - Add RunEndEncodedScalarType - Fixes apache#32543 - Fixes apache#32544 - Fixes apache#32688 - Fixes apache#32731 - Fixes apache#32772 - Fixes apache#32774 * Closes: apache#32104 Lead-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Co-authored-by: Tobias Zagorni <tobias@zagorni.eu> Signed-off-by: Matt Topol <zotthewizard@gmail.com>

ursabot · 2023-02-17T18:48:03Z

Benchmark runs are scheduled for baseline = 157b8f5 and contender = 1264e40. 1264e40 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.58% ⬆️0.03%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.41% ⬆️0.03%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 1264e409 ec2-t3-xlarge-us-east-2
[Finished] 1264e409 test-mac-arm
[Finished] 1264e409 ursa-i9-9960x
[Finished] 1264e409 ursa-thinkcentre-m75q
[Finished] 157b8f55 ec2-t3-xlarge-us-east-2
[Finished] 157b8f55 test-mac-arm
[Finished] 157b8f55 ursa-i9-9960x
[Finished] 157b8f55 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

…pache#33641) This PR gathers work from multiple PRs that can be closed after this one is merged: - Closes apache#13752 - Closes apache#13754 - Closes apache#13842 - Closes apache#13882 - Closes apache#13916 - Closes apache#14063 - Closes apache#13970 And the issues associated with those PRs can also be closed: - Fixes apache#20350 - Add RunEndEncodedScalarType - Fixes apache#32543 - Fixes apache#32544 - Fixes apache#32688 - Fixes apache#32731 - Fixes apache#32772 - Fixes apache#32774 * Closes: apache#32104 Lead-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Co-authored-by: Tobias Zagorni <tobias@zagorni.eu> Signed-off-by: Matt Topol <zotthewizard@gmail.com>

Related issue number -------------------- See also: - Homebrew/homebrew-core#129859 - apache/arrow#33608 - apache/arrow#33641 Signed-off-by: Tao He <linzhu.ht@alibaba-inc.com>

github-actions bot added Component: C++ Component: Parquet Component: Python labels Jan 12, 2023

felipecrv force-pushed the ree branch 3 times, most recently from 6156a9f to 1d4de2a Compare January 17, 2023 17:07

felipecrv force-pushed the ree branch 3 times, most recently from dd7fd09 to f5d88b9 Compare January 17, 2023 23:51

pitrou reviewed Jan 18, 2023

View reviewed changes

felipecrv commented Jan 18, 2023

View reviewed changes

cpp/src/arrow/CMakeLists.txt Outdated Show resolved Hide resolved

felipecrv commented Jan 18, 2023

View reviewed changes

cpp/src/arrow/array.h Outdated Show resolved Hide resolved

felipecrv force-pushed the ree branch from 75f058e to 420d1e5 Compare January 23, 2023 23:24

felipecrv force-pushed the ree branch from 420d1e5 to bed2c9a Compare February 1, 2023 12:17

felipecrv force-pushed the ree branch 2 times, most recently from 7ff1ce7 to fab46a5 Compare February 3, 2023 14:41

felipecrv force-pushed the ree branch 8 times, most recently from 6396a80 to 2811015 Compare February 7, 2023 19:45

felipecrv and others added 5 commits February 15, 2023 16:01

REE Array: Rewrite the RunEndEncodedBuilder

a75dc18

REE Array: Rewrite MergedRunsIterator

5f602ea

REE Array: Implement Concatenate + tests

3209950

Lead-authored-by: Tobias Zagorni <tobias@zagorni.eu> Co-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

REE Array: ARROW_CHECK value and run_end types

c404a83

REE Array: Check values and run_ends lengths

88b209b

felipecrv force-pushed the ree branch from 94fb574 to dc41152 Compare February 15, 2023 19:02

felipecrv force-pushed the ree branch from dc41152 to 9f3e699 Compare February 15, 2023 20:14

felipecrv mentioned this pull request Feb 15, 2023

GH-34206: [C++] Don't let jemalloc defines affect unity builds #34185

Merged

zeroshade approved these changes Feb 16, 2023

View reviewed changes

lidavidm reviewed Feb 16, 2023

View reviewed changes

felipecrv added 8 commits February 16, 2023 20:50

Remove NOTE that made it sound like I was going to fix a lot of code :)

4b926b7

Make param order of utils consistent

87e8a8a

Fix test name

b89d8fd

REE Scalar: Check type in ctor

59f1791

REE Array: Remove length-sum check against INT_MAX in Concatenate

4cae325

REE Builder: Check for overflow in CloseRun

deb7c30

Debug-check index argument to FindPhysicalIndex

e2926e2

Use HasValidityBimap in tests

0fbf464

lidavidm approved these changes Feb 17, 2023

View reviewed changes

zeroshade merged commit 1264e40 into apache:main Feb 17, 2023

felipecrv deleted the ree branch February 17, 2023 18:50

sighingnow mentioned this pull request May 8, 2023

Adapt to arrow-11/12 (VisitTypeInline & Run-End encoded data) v6d-io/v6d#1362

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-32104: [C++] Add support for Run-End encoded data to Arrow #33641

GH-32104: [C++] Add support for Run-End encoded data to Arrow #33641

felipecrv commented Jan 12, 2023 •

edited

Loading

github-actions bot commented Jan 12, 2023

github-actions bot commented Jan 12, 2023

github-actions bot commented Jan 17, 2023

pitrou left a comment

pitrou Jan 18, 2023

lidavidm Jan 18, 2023

pitrou left a comment

github-actions bot commented Feb 1, 2023

felipecrv commented Feb 3, 2023

felipecrv commented Feb 16, 2023

zeroshade left a comment

lidavidm left a comment

felipecrv commented Feb 17, 2023 •

edited

Loading

lidavidm commented Feb 17, 2023

ursabot commented Feb 17, 2023

GH-32104: [C++] Add support for Run-End encoded data to Arrow #33641

GH-32104: [C++] Add support for Run-End encoded data to Arrow #33641

Conversation

felipecrv commented Jan 12, 2023 • edited Loading

github-actions bot commented Jan 12, 2023

github-actions bot commented Jan 12, 2023

github-actions bot commented Jan 17, 2023

pitrou left a comment

Choose a reason for hiding this comment

pitrou Jan 18, 2023

Choose a reason for hiding this comment

lidavidm Jan 18, 2023

Choose a reason for hiding this comment

pitrou left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 1, 2023

felipecrv commented Feb 3, 2023

felipecrv commented Feb 16, 2023

zeroshade left a comment

Choose a reason for hiding this comment

lidavidm left a comment

Choose a reason for hiding this comment

felipecrv commented Feb 17, 2023 • edited Loading

lidavidm commented Feb 17, 2023

ursabot commented Feb 17, 2023

felipecrv commented Jan 12, 2023 •

edited

Loading

felipecrv commented Feb 17, 2023 •

edited

Loading