GH-37378: [C++] Add A Dictionary Compaction Function For DictionaryArray by R-JunmingChen · Pull Request #37418 · apache/arrow · GitHub

R-JunmingChen · 2023-08-28T09:25:45Z

Rationale for this change

A Dictionary Compaction Function for DictionaryArray is supported.

What changes are included in this PR?

Add a Function for Dictionary Compaction

Are these changes tested?

Yes

Are there any user-facing changes?
No

Closes: [C++] Add a Dictionary Compaction Function For DictionaryArray #37378

github-actions · 2023-08-28T09:26:20Z

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

bkietz · 2023-10-10T11:32:25Z

+        dict_used[current_index] = true;
+        dict_used_count++;
+
+        if (dict_used_count == dict_length) {


Checking here enables skipping the rest of the dictionary, which is good. However I think it'd also be useful to detect usage of only a slice of the dictionary. If you'd prefer not to handle that in this PR, please write a follow up issue

Hi, @bkietz, do you mean if we find it just use only a slice of dictionay, we use slice() instead of Take? I prefer to leave it as an new issue. Since we have another PR which is wating for this PR to be merged.

Can we just use Slice outside the Compact to handling this?

I write a follow up issue #38247 to handle the optimization

Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>

mapleFU

Rest LGTM, thanks!

mapleFU · 2023-10-11T08:40:55Z

+
+    type = boolean();
+    dict_type = dictionary(index_type, type);
+


Should we add a test for invalid input type?

No. We can't create a DictionaryArray with an invalid index type.

Oh I see a

if (data->type->id() != Type::DICTIONARY) { return Status::TypeError("Expected dictionary type"); }

Here, but seems it cannot be called

bkietz

LGTM, thanks for working on this!

R-JunmingChen · 2023-10-11T14:57:31Z

Strange. All C++ tests are canceled in CI.

bkietz · 2023-10-11T17:29:27Z

CI failures look unrelated. Thanks!

conbench-apache-arrow · 2023-10-15T09:13:24Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 73454b7.

There were 4 benchmark results indicating a performance regression:

Commit Run on ursa-i9-9960x at 2023-10-12 15:57:42Z
- tpch (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-01, scale_factor=1
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-06, scale_factor=1
and 2 more (see the report linked below)

The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.

…naryArray (apache#37418) ### Rationale for this change A Dictionary Compaction Function for DictionaryArray is supported. ### What changes are included in this PR? Add a Function for Dictionary Compaction ### Are these changes tested? Yes Are there any user-facing changes? No * Closes: apache#37378 Lead-authored-by: Junming Chen <junming.chen.r@outlook.com> Co-authored-by: Ben Harkins <60872452+benibus@users.noreply.github.com> Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com> Signed-off-by: Benjamin Kietzman <bengilgit@gmail.com>

compaction init

1dadf59

github-actions Bot added Component: C++ awaiting review Awaiting review labels Aug 28, 2023

R-JunmingChen added 4 commits August 28, 2023 17:40

fix bug

cf474f5

add finsih draft

aec9ca2

lint

e522841

compaction draft done

9853df3

R-JunmingChen marked this pull request as ready for review August 31, 2023 15:49

R-JunmingChen added 12 commits September 1, 2023 09:07

lint

23eed73

optimized

d1af639

Update vector_hash.cc

83a38e1

Update vector_hash.cc

dafd8c8

Update vector_hash.cc

cf29aae

Update vector_hash.cc

3fa7abc

lint

74e4fb1

lint

ef1f335

add a basic test

354b5b0

empty situation

38a7bcc

full boolean test

93f4029

optimize

1fe45d0

benibus reviewed Sep 5, 2023

View reviewed changes

Comment thread cpp/src/arrow/compute/registry.cc Outdated

github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Sep 5, 2023

registry

46959c2

R-JunmingChen requested a review from benibus September 6, 2023 01:34

R-JunmingChen added 4 commits September 7, 2023 22:47

move to vector_dictionary

a3dbf0f

rename

c6fd4f2

refine doc

73cc864

lint

092a38c

Vistor

a7d5c60

github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 10, 2023

R-JunmingChen requested a review from bkietz October 10, 2023 03:26

bkietz requested changes Oct 10, 2023

View reviewed changes

github-actions Bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Oct 10, 2023

Update cpp/src/arrow/array/array_dict.cc

168a891

Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>

github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 11, 2023

R-JunmingChen and others added 3 commits October 11, 2023 08:39

Update cpp/src/arrow/array/array_dict.cc

0cffac0

Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>

unsafe append

c1a4635

Delete function

1665144

R-JunmingChen requested a review from bkietz October 11, 2023 08:06

mapleFU reviewed Oct 11, 2023

View reviewed changes

bkietz approved these changes Oct 11, 2023

View reviewed changes

github-actions Bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Oct 11, 2023

out of bound

4562d0f

mapleFU approved these changes Oct 11, 2023

View reviewed changes

bkietz merged commit 73454b7 into apache:main Oct 11, 2023

bkietz removed the awaiting merge Awaiting merge label Oct 11, 2023

github-actions Bot added the awaiting committer review Awaiting committer review label Oct 12, 2023

Conversation

R-JunmingChen commented Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Uh oh!

github-actions Bot commented Aug 28, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkietz Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

R-JunmingChen Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mapleFU Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

R-JunmingChen Oct 12, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mapleFU left a comment

Choose a reason for hiding this comment

Uh oh!

mapleFU Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

R-JunmingChen Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mapleFU Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

bkietz left a comment

Choose a reason for hiding this comment

Uh oh!

R-JunmingChen commented Oct 11, 2023

Uh oh!

bkietz commented Oct 11, 2023

Uh oh!

conbench-apache-arrow Bot commented Oct 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

R-JunmingChen commented Aug 28, 2023 •

edited

Loading

R-JunmingChen Oct 11, 2023 •

edited

Loading

R-JunmingChen Oct 11, 2023 •

edited

Loading