GH-43911: [C++] Compute Row: ListKeyEncoder Supports #43912

mapleFU · 2024-09-02T15:45:46Z

Rationale for this change

Add ListKeyEncoder supports in RowEncoder

What changes are included in this PR?

Add ListKeyEncoder supports in RowEncoder
Change RowEncoder::Init to return Status

Are these changes tested?

Yes

Are there any user-facing changes?

Currently not, they're internal interfaces

GitHub Issue: [C++] Compute: KeyEncoder support List Type #43911

github-actions · 2024-09-02T15:46:13Z

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

mapleFU · 2024-09-02T15:46:19Z

cpp/src/arrow/compute/row/row_encoder_internal.cc

@@ -29,6 +29,51 @@ using internal::FirstTimeBitmapWriter;
 namespace compute {
 namespace internal {

+Result<std::shared_ptr<KeyEncoder>> MakeKeyEncoder(const TypeHolder& column_type, std::shared_ptr<ExtensionType>* extension_type, MemoryPool* pool) {


This can also return unique_ptr here. I didn't see the purpose a shared_ptr being used

Also this function is extracted from RowEncoder

mapleFU · 2024-09-08T10:52:26Z

cpp/src/arrow/compute/row/row_encoder_internal.h

@@ -269,6 +272,190 @@ struct ARROW_EXPORT NullKeyEncoder : KeyEncoder {
  }
 };

+template <typename ListType>
+struct ARROW_EXPORT ListKeyEncoder : KeyEncoder {


Wonder should I put this into .cc since it requires a lot

Yes, please do. It would be nice to hide most contents from this file into the corresponding .cc

mapleFU · 2024-09-08T10:53:37Z

cpp/src/arrow/compute/row/row_encoder_internal.h

+      // AddLength for each list
+      std::vector<int32_t> child_lengthes;


This is for AddLength, if lots of value being used, AddLength call AddLength(child_lengthes.data(), length) rather than call with length 1

mapleFU · 2024-09-08T10:53:48Z

cpp/src/arrow/compute/row/row_encoder_internal.h

+      if (list_scalar.is_valid && list_scalar.value->length() > 0) {
+        auto element_count = static_cast<int32_t>(list_scalar.value->length());
+        // Counting the size of the encoded list
+        std::vector<int32_t> child_lengthes(element_count, 0);


mapleFU · 2024-09-08T10:54:37Z

cpp/src/arrow/compute/row/row_encoder_internal.h

+        RETURN_NOT_OK(
+            this->element_encoder_->Encode(ExecValue{tmp_child_data}, 1, &encoded_ptr));


This part is a bit tricky, since Encode don't has interface for "encode 1 element", so this call Encode(1).

mapleFU · 2024-09-08T10:55:07Z

cpp/src/arrow/compute/row/row_encoder_internal.h

+      ARROW_ASSIGN_OR_RAISE(auto element_array, ::arrow::Concatenate(child_datas, pool));
+      element_data = element_array->data();
+    } else {
+      // If there are no elements, we need to create an empty array


This requires an "empty" ArrayData

mapleFU · 2024-09-08T10:58:07Z

@pitrou @zanmato1984 @felipecrv I've written a basic impl, the performance here might be bad but this implmenet the basic logic. Would you mind take a look?

(I'll be out for vocation in 9.14 - 9.21, so maybe late response later)

mapleFU · 2024-09-08T11:00:10Z

cpp/src/arrow/compute/row/row_encoder_internal.h

+    auto raw_offsets = offset_buf->mutable_span_as<Offset>();
+    Offset element_sum = 0;
+    raw_offsets[0] = 0;
+    std::vector<std::shared_ptr<Array>> child_datas;


This is a bit tricky, it always decode 1 from child, so we'll have "element-size" Array here...

mapleFU · 2024-09-08T11:05:29Z

I think with something like a callback:

struct ARROW_EXPORT KeyEncoder {
  virtual Result<std::shared_ptr<ArrayData>> DecodeWithBuilder(ArrayBuilder* builder) = 0;
}

I didn't find efficient interface for encoder, I may go through other code for help

pitrou

Incomplete review for now

pitrou · 2024-09-17T15:33:07Z

cpp/src/arrow/acero/hash_join_node_test.cc

@@ -514,7 +514,7 @@ std::vector<std::shared_ptr<Array>> GenRandomUniqueRecords(
    val_types.push_back(result[i]->type());
  }
  RowEncoder encoder;
-  encoder.Init(val_types, ctx);
+  auto s = encoder.Init(val_types, ctx);


Please at least use DCHECK_OK.

pitrou · 2024-09-17T15:33:33Z

cpp/src/arrow/compute/row/row_encoder_internal.h

@@ -18,10 +18,13 @@
 #pragma once

 #include <cstdint>
+#include <iostream>


For debug, will remove

pitrou · 2024-09-17T15:33:44Z

cpp/src/arrow/compute/row/row_encoder_internal.h

@@ -269,6 +272,190 @@ struct ARROW_EXPORT NullKeyEncoder : KeyEncoder {
  }
 };

+template <typename ListType>
+struct ARROW_EXPORT ListKeyEncoder : KeyEncoder {


Yes, please do. It would be nice to hide most contents from this file into the corresponding .cc

pitrou · 2024-09-17T15:36:24Z

cpp/src/arrow/compute/row/row_encoder_internal.h

@@ -269,6 +272,190 @@ struct ARROW_EXPORT NullKeyEncoder : KeyEncoder {
  }
 };

+template <typename ListType>
+struct ARROW_EXPORT ListKeyEncoder : KeyEncoder {


Can you add a comment explaining how the encoding looks like?

Ah, I see you added a comment below.

pitrou · 2024-09-17T15:41:09Z

cpp/src/arrow/compute/row/row_encoder_internal.h

+      VisitBitBlocksVoid(
+          validity, data.array.offset, data.array.length,
+          [&](int64_t i) {
+            ARROW_UNUSED(i);


pitrou · 2024-09-17T15:41:23Z

cpp/src/arrow/compute/row/row_encoder_internal.h

+          validity, data.array.offset, data.array.length,
+          [&](int64_t i) {
+            ARROW_UNUSED(i);
+            child_lengthes.clear();


No need to call clear as you're calling resize below.

pitrou · 2024-09-17T15:41:30Z

cpp/src/arrow/compute/row/row_encoder_internal.h

+      const uint8_t* validity = data.array.buffers[0].data;
+      const auto* offsets = data.array.GetValues<Offset>(1);
+      // AddLength for each list
+      std::vector<int32_t> child_lengthes;


Suggested change

std::vector<int32_t> child_lengthes;

std::vector<int32_t> child_lengths;

pitrou · 2024-09-17T15:44:47Z

cpp/src/arrow/compute/row/row_encoder_internal.h

+      for (int64_t i = 0; i < child_array.length; i++) {
+        ArraySpan tmp_child_data(child_array);
+        tmp_child_data.SetSlice(child_array.offset + i, 1);
+        RETURN_NOT_OK(
+            this->element_encoder_->Encode(ExecValue{tmp_child_data}, 1, &encoded_ptr));
+      }


Why do it one element at a time? Why not instead:

Suggested change

for (int64_t i = 0; i < child_array.length; i++) {

ArraySpan tmp_child_data(child_array);

tmp_child_data.SetSlice(child_array.offset + i, 1);

RETURN_NOT_OK(

this->element_encoder_->Encode(ExecValue{tmp_child_data}, 1, &encoded_ptr));

}

RETURN_NOT_OK(

this->element_encoder_->Encode(ExecValue{child_array}, child_array.length, &encoded_ptr));

pitrou · 2024-09-17T15:44:58Z

cpp/src/arrow/compute/row/row_encoder_internal.h

+      RETURN_NOT_OK(VisitBitBlocks(
+          validity, data.array.offset, data.array.length,
+          [&](int64_t i) {
+            ARROW_UNUSED(i);


pitrou · 2024-09-17T15:46:49Z

cpp/src/arrow/compute/row/row_encoder_internal.h

+        for (int64_t i = 0; i < batch_length; i++) {
+          RETURN_NOT_OK(handle_valid_value(span));
+        }
+      } else {
+        for (int64_t i = 0; i < batch_length; i++) {
+          handle_null_value();
+        }


You could instead call handle_valid_value or handle_null_value once and then memcpy the result batch_length - 1 times.

zanmato1984

A small simplification.

zanmato1984 · 2024-09-18T06:48:13Z

cpp/src/arrow/compute/row/row_encoder_internal.h

+      auto& encoded_ptr = *encoded_bytes++;
+      *encoded_ptr++ = kNullByte;
+      util::SafeStore(encoded_ptr, static_cast<Offset>(0));
+      encoded_ptr += sizeof(Offset);


Suggested change

encoded_ptr += sizeof(Offset);

encoded_ptr += sizeof(Offset);

return Status::OK();

zanmato1984 · 2024-09-18T06:49:54Z

cpp/src/arrow/compute/row/row_encoder_internal.h

+          [&]() {
+            handle_null_value();
+            return Status::OK();
+          }));


Suggested change

[&]() {

handle_null_value();

return Status::OK();

}));

handle_null_value));

Compute: ListKeyEncoder interfaces

f5f0f33

github-actions bot added the Component: C++ label Sep 2, 2024

mapleFU commented Sep 2, 2024

View reviewed changes

github-actions bot added awaiting review Awaiting review awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Sep 2, 2024

mapleFU added 5 commits September 3, 2024 00:05

try to make ci happy

7ed5df4

Merge branch 'main' into support-list-in-row-encoder

9aaff83

minor-refactor

5055ef0

basic impl

222909a

Merge branch 'main' into support-list-in-row-encoder

4e8325e

mapleFU marked this pull request as ready for review September 8, 2024 10:51

mapleFU requested a review from westonpace as a code owner September 8, 2024 10:51

basic impl

72705a9

mapleFU force-pushed the support-list-in-row-encoder branch from 24dd410 to 72705a9 Compare September 8, 2024 10:55

mapleFU commented Sep 8, 2024

View reviewed changes

mapleFU requested review from zanmato1984 and pitrou September 12, 2024 07:04

pitrou requested changes Sep 17, 2024

View reviewed changes

zanmato1984 reviewed Sep 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-43911: [C++] Compute Row: ListKeyEncoder Supports #43912

GH-43911: [C++] Compute Row: ListKeyEncoder Supports #43912

mapleFU commented Sep 2, 2024 •

edited

Loading

github-actions bot commented Sep 2, 2024

mapleFU Sep 2, 2024 •

edited

Loading

mapleFU Sep 8, 2024

pitrou Sep 17, 2024

mapleFU Sep 8, 2024

mapleFU Sep 8, 2024

mapleFU Sep 8, 2024

mapleFU Sep 8, 2024

mapleFU commented Sep 8, 2024

mapleFU Sep 8, 2024

mapleFU commented Sep 8, 2024

pitrou left a comment

pitrou Sep 17, 2024

pitrou Sep 17, 2024

mapleFU Sep 23, 2024

pitrou Sep 17, 2024

pitrou Sep 17, 2024

pitrou Sep 17, 2024

pitrou Sep 17, 2024

pitrou Sep 17, 2024

pitrou Sep 17, 2024

pitrou Sep 17, 2024

pitrou Sep 17, 2024

pitrou Sep 17, 2024

zanmato1984 left a comment

zanmato1984 Sep 18, 2024

zanmato1984 Sep 18, 2024

		// AddLength for each list
		std::vector<int32_t> child_lengthes;

		RETURN_NOT_OK(
		this->element_encoder_->Encode(ExecValue{tmp_child_data}, 1, &encoded_ptr));

	std::vector<int32_t> child_lengthes;
	std::vector<int32_t> child_lengths;

	encoded_ptr += sizeof(Offset);
	encoded_ptr += sizeof(Offset);
	return Status::OK();

GH-43911: [C++] Compute Row: ListKeyEncoder Supports #43912

Are you sure you want to change the base?

GH-43911: [C++] Compute Row: ListKeyEncoder Supports #43912

Conversation

mapleFU commented Sep 2, 2024 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Sep 2, 2024

mapleFU Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapleFU commented Sep 8, 2024

Choose a reason for hiding this comment

mapleFU commented Sep 8, 2024

pitrou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanmato1984 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapleFU commented Sep 2, 2024 •

edited

Loading

mapleFU Sep 2, 2024 •

edited

Loading