Add decrease interface for aggregation by xzhangxian1008 · Pull Request #9737 · pingcap/tiflash

xzhangxian1008 · 2024-12-20T03:13:23Z

What problem does this PR solve?

Issue Number: ref #7376

Problem Summary:

What is changed and how it works?

In order to support aggregation in window function, we need to add some interfaces for aggregation. In this pr, we add `decrease` interface for aggregation.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

windtalker · 2025-01-15T06:13:59Z

dbms/src/AggregateFunctions/AggregateFunctionMinMaxWindow.h

+        saved_values.erase(iter);
+    }
+
+    void changeIfLess(const IColumn & column, size_t row_num, Arena *)


I think you can define an add method, both changeIfLess and changeIfGreater can call add

I think you can define an add method, both changeIfLess and changeIfGreater can call add

done

windtalker · 2025-01-15T06:22:12Z

dbms/src/AggregateFunctions/AggregateFunctionMinMaxWindow.h

+            if constexpr (is_min)
+                iter = saved_values.begin();
+            else
+                iter = std::prev(saved_values.end());


why not just use iter = saved_values.rbegin()?

why not just use iter = saved_values.rbegin()?

done

windtalker · 2025-01-15T06:23:34Z

dbms/src/AggregateFunctions/AggregateFunctionMinMaxWindow.h

+        {}
+
+        // TODO use std::string is inefficient
+        std::string value;


why not use StringRef or std::string_view

why not use StringRef or std::string_view

Using StringRef now.

windtalker · 2025-01-15T06:32:52Z

dbms/src/AggregateFunctions/AggregateFunctionNull.h


 public:
-    explicit AggregateFunctionNullBase(AggregateFunctionPtr nested_function_)
+    explicit AggregateFunctionNullBase(AggregateFunctionPtr nested_function_, bool need_counter = false)


why need_counter is still needed?

why need_counter is still needed?

deleted

windtalker · 2025-01-16T05:25:37Z

dbms/src/AggregateFunctions/AggregateFunctionMinMaxWindow.h

+            , collator(collator_)
+        {}
+
+        // TODO use StringRef is inefficient


what is the todo mean, is there any more efficient implement here?

what is the todo mean, is there any more efficient implement here?

It can be deleted now.

windtalker · 2025-01-16T05:33:22Z

dbms/src/AggregateFunctions/AggregateFunctionMinMaxWindow.h

+    mutable multiset saved_values;
+    TiDB::TiDBCollatorPtr collator{};
+
+    void saveValue(StringRef value) { saved_values.insert(StringWithCollator(value, collator)); }


Suggested change

void saveValue(StringRef value) { saved_values.insert(StringWithCollator(value, collator)); }

void saveValue(StringRef & value) { saved_values.insert(StringWithCollator(value, collator)); }

windtalker · 2025-01-16T05:43:01Z

dbms/src/AggregateFunctions/AggregateFunctionNull.h

+
+    switch (prefix_size)
+    {
+    case 1:


why not just use an array like [sizeof(uint64), sizeof(uint64), sizeof(uint64), 9, sizeof(uint64), 10, 12, 14], so the result_prefix_size = array[prefix_size], or use a static array so it can be init at runtime without assuming sizeof(uint64) == 8

why not just use an array like [sizeof(uint64), sizeof(uint64), sizeof(uint64), 9, sizeof(uint64), 10, 12, 14], so the result_prefix_size = array[prefix_size], or use a static array so it can be init at runtime without assuming sizeof(uint64) == 8

done

windtalker · 2025-01-16T05:47:09Z

dbms/src/AggregateFunctions/AggregateFunctionNull.h

 };

+// Make the prefix_size >= sizeof(UInt64) and could fit the align size
+inline size_t enlarge_prefix_size(size_t prefix_size) noexcept


Looks like as the function name, it should be enlargePrefixSize

Looks like as the function name, it should be enlargePrefixSize

done

windtalker · 2025-01-16T06:43:19Z

dbms/src/Common/AlignedBuffer.cpp

+            fmt::format("Cannot allocate memory (posix_memalign), size: {}, alignment: {}.", size, alignment),
+            ErrorCodes::CANNOT_ALLOCATE_MEMORY,
+            res);
+    buf = new_buf;


The buf is always nullptr when this function is called?

The buf is always nullptr when this function is called?

yes

windtalker · 2025-01-16T06:46:25Z

dbms/src/Common/AlignedBuffer.h

+public:
+    AlignedBuffer() = default;
+    AlignedBuffer(size_t size, size_t alignment);
+    AlignedBuffer(AlignedBuffer && old) noexcept { std::swap(buf, old.buf); }


This function seems useless, because there is no guarantee that the alignment is the same?

This function seems useless, because there is no guarantee that the alignment is the same?

deleted

windtalker · 2025-01-16T11:45:23Z

dbms/src/AggregateFunctions/AggregateFunctionNull.h

+inline size_t enlargePrefixSize(size_t prefix_size) noexcept
 {
    assert(prefix_size != 0);
+    static_assert((sizeof(prefix_size_look_up_table) / sizeof(UInt64)) == 7);


asdd another static_assert to make sure sizeof(UInt64) == 8?

asdd another static_assert to make sure sizeof(UInt64) == 8?

done

windtalker

lgtm

ti-chi-bot · 2025-01-17T02:16:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: guo-shaoge, windtalker

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [guo-shaoge,windtalker]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2025-01-17T02:17:00Z

[LGTM Timeline notifier]

Timeline:

2025-01-10 05:51:18.42432135 +0000 UTC m=+505621.713153055: ☑️ agreed by guo-shaoge.
2025-01-17 02:16:59.579515284 +0000 UTC m=+257691.034561430: ☑️ agreed by windtalker.

dbms/src/AggregateFunctions/AggregateFunctionNull.h

xzhangxian1008 · 2025-03-11T01:40:51Z

/cherrypick release-8.5-20250310-v8.5.1

ref pingcap#7376 In order to support aggregation in window function, we need to add some interfaces for aggregation. In this pr, we add `decrease` interface for aggregation. Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

ti-chi-bot · 2025-03-11T01:41:58Z

@xzhangxian1008: new pull request created to branch release-8.5-20250310-v8.5.1: #9974.

Details

In response to this:

/cherrypick release-8.5-20250310-v8.5.1

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ref #7376 In order to support aggregation in window function, we need to add some interfaces for aggregation. In this pr, we add `decrease` interface for aggregation. Co-authored-by: xzhangxian1008 <xzhangxian@foxmail.com>

xzhangxian1008 added 19 commits November 26, 2024 16:58

init

3f80eed

add gtest

171fd46

add ft

943c578

refine test framework

e0b05a2

fix compilation phase

75d2f12

init

7022607

codes done, need tests

b011bfa

save

8628a2e

add tests

db4e399

add sum tests

a9360da

refine tests

e3c31bd

format

e3b9756

fix bugs

a159857

refine test

0d4401d

fix tests

9ff2bd3

add test for string type

a9aa879

add test for SingleValueDataGeneric type

bf652bf

tweaking

61eaef0

Merge branch 'master' into wagg

6017780

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 20, 2024

xzhangxian1008 added 9 commits December 20, 2024 11:41

remove something

2358efb

revoke

3227b50

add AlignedBuffer

b2772ea

remove something

37f4cc5

tweaking

dd52c9f

remove useless change

6379b0f

fix ci

9e41b3a

fix ci

e6adcbf

fix ut

b84c911

xzhangxian1008 added 2 commits January 14, 2025 13:04

address some comments

480a77f

address comments

c2e9975

xzhangxian1008 requested a review from windtalker January 14, 2025 06:15

windtalker reviewed Jan 15, 2025

View reviewed changes

addresscomments

c0e25b2

xzhangxian1008 requested a review from windtalker January 15, 2025 14:18

xzhangxian1008 added 2 commits January 15, 2025 22:21

Empty-Commit

8bc36ae

tweaking

3591ed8

windtalker reviewed Jan 16, 2025

View reviewed changes

xzhangxian1008 added 2 commits January 16, 2025 17:31

address comments

0de711b

tweaking

e1bbc8f

windtalker reviewed Jan 16, 2025

View reviewed changes

address comment

d5fa58e

xzhangxian1008 requested a review from windtalker January 17, 2025 01:00

windtalker approved these changes Jan 17, 2025

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jan 17, 2025

ti-chi-bot bot added 2 commits January 17, 2025 02:33

Merge branch 'master' into decrease-agg

6033e42

Merge branch 'master' into decrease-agg

078d160

ti-chi-bot bot merged commit 8dade17 into pingcap:master Jan 17, 2025

wuhuizuo reviewed Jan 17, 2025

View reviewed changes

dbms/src/AggregateFunctions/AggregateFunctionNull.h Show resolved Hide resolved

xzhangxian1008 deleted the decrease-agg branch January 17, 2025 05:38

ti-chi-bot mentioned this pull request Mar 11, 2025

Add decrease interface for aggregation (#9737) #9974

Merged

12 tasks

JaySon-Huang mentioned this pull request Mar 12, 2025

Fix compilation errors with unused variables #9793

Merged

12 tasks

	void saveValue(StringRef value) { saved_values.insert(StringWithCollator(value, collator)); }
	void saveValue(StringRef & value) { saved_values.insert(StringWithCollator(value, collator)); }

Conversation

xzhangxian1008 commented Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

windtalker left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Jan 17, 2025

Uh oh!

ti-chi-bot bot commented Jan 17, 2025

[LGTM Timeline notifier]

Uh oh!

Uh oh!

xzhangxian1008 commented Mar 11, 2025

Uh oh!

ti-chi-bot commented Mar 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

xzhangxian1008 commented Dec 20, 2024 •

edited

Loading