[Enhancement] Support aggregate function map_agg. #51967

Jcnessss · 2024-10-16T07:07:07Z

Why I'm doing:

Map_agg is an aggregate function to generate map value, which is frequently used in Trino. To support migrate from Trino to Starrocks, develop the same function in Starrocks.

Related issue #40894

What I'm doing:

I tried 3 ways to develop this function:

Using template: key type as KT, value type as VT, in which case neither the key or value could be complex type.
Not using template: Both key and value could be complex type, however, we need to call map_column#remove_duplicated_keys, and there is a triple loop in it, which will highly decrease the performance of map_agg.
Using template: key type as KT, in which case the value could be complex type.

I think the 3rd one will be the best one, and the behavior is just like Trino.

What type of PR is this:

Does this PR entail a change in behavior?

Yes, this PR will result in a change in behavior.
No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

Interface/UI changes: syntax, type conversion, expression evaluation, display information
Parameter changes: default values, similar parameters but with different default values
Policy changes: use new policy to replace old one, functionality automatically enabled
Feature removed
Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

I have added test cases for my bug fix or my new feature
This pr needs user documentation (for new or modified features or behaviors)
- I have added documentation for my new feature or new function
This is a backport pr

Bugfix cherry-pick branch check:

be/src/exprs/agg/map_agg.h

stdpain · 2024-10-16T07:42:33Z

be/src/exprs/agg/map_agg.h

+    using KeyType = typename SliceHashSet::key_type;
+
+    MyHashMap hash_map;
+    void update(MemPool* mem_pool,


It seems other system implement has a max limit for map_agg

I'm not sure. It seems that the map_agg of Trino does not have a limit, all my implement is simulating the one in Trino.

Jcnessss · 2024-10-16T09:31:32Z

@stdpain Thanks for the review again! I have changed the code according to your review. Could you please help me review again?

be/src/exprs/agg/map_agg.h

Jcnessss · 2024-10-17T07:29:08Z

@stdpain Thanks for all the reviews!

Jcnessss · 2024-10-18T08:40:03Z

@stdpain Hi, I'm sorry that I forgot to commit a part of FE codes. Could you please review this again?

stdpain · 2024-10-22T03:23:48Z

https://github.com/Mergifyio rebase main

mergify · 2024-10-22T03:23:58Z

rebase main

❌ Unable to rebase: user `stdpain` is unknown.

Please make sure stdpain has logged in Mergify dashboard.

stdpain · 2024-10-22T06:09:42Z

https://github.com/Mergifyio rebase main

mergify · 2024-10-22T06:10:12Z

rebase main

✅ Branch has been successfully rebased

Jcnessss · 2024-10-28T03:37:46Z

@stdpain Hello, the CI passed(The failure in admit test seems has nothing with this PR.) and I have changed the implement way of the value in map, using a column to store values instead in order to keep the reference of slices.
Could you please look at this again?

stdpain · 2024-10-30T12:39:14Z

be/src/exprs/agg/map_agg.h

+            key_column->append(*src[0], i, 1);
+            value_column->append(*src[1], i, 1);
+            offsets.push_back(offsets.back() + 1);
+        }


Suggested change

}

}

if (dst->is_nullable()) {

down_cast<NullableColumn*>(dst)->null_column_data().resize(dst->size() + chunk_size);

}

Thanks for the review! Done.

Signed-off-by: Song Jiacheng <[email protected]>

sonarcloud · 2024-10-31T03:03:11Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

github-actions · 2024-10-31T04:28:45Z

[Java-Extensions Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

github-actions · 2024-10-31T04:31:41Z

[FE Incremental Coverage Report]

✅ pass : 19 / 19 (100.00%)

file detail

	path	covered_line	new_line	coverage	not_covered_line_detail
🔵	com/starrocks/catalog/FunctionSet.java	17	17	100.00%	[]
🔵	com/starrocks/sql/analyzer/PolymorphicFunctionAnalyzer.java	2	2	100.00%	[]

github-actions · 2024-10-31T04:39:17Z

[BE Incremental Coverage Report]

✅ pass : 70 / 84 (83.33%)

file detail

	path	covered_line	new_line	coverage	not_covered_line_detail
🔵	be/src/exprs/agg/map_agg.h	61	75	81.33%	[55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 109, 110, 111, 157]
🔵	be/src/exprs/agg/factory/aggregate_resolver_avg.cpp	9	9	100.00%	[]

Jcnessss · 2024-11-08T02:31:22Z

Thanks for all the reviews!

Jcnessss requested review from a team as code owners October 16, 2024 07:07

mergify bot assigned Jcnessss Oct 16, 2024

stdpain reviewed Oct 16, 2024

View reviewed changes

be/src/exprs/agg/map_agg.h Outdated Show resolved Hide resolved

stdpain reviewed Oct 16, 2024

View reviewed changes

be/src/exprs/agg/map_agg.h Outdated Show resolved Hide resolved

stdpain reviewed Oct 16, 2024

View reviewed changes

be/src/exprs/agg/map_agg.h Show resolved Hide resolved

stdpain reviewed Oct 16, 2024

View reviewed changes

Jcnessss force-pushed the map_agg branch from f124edd to 317187b Compare October 16, 2024 09:30

Jcnessss requested a review from stdpain October 16, 2024 11:18

stdpain reviewed Oct 17, 2024

View reviewed changes

be/src/exprs/agg/map_agg.h Show resolved Hide resolved

stdpain previously approved these changes Oct 17, 2024

View reviewed changes

Jcnessss dismissed stdpain’s stale review via 644a6f9 October 17, 2024 07:27

Jcnessss force-pushed the map_agg branch from 317187b to 644a6f9 Compare October 17, 2024 07:27

Jcnessss force-pushed the map_agg branch from 644a6f9 to 5ea0e64 Compare October 18, 2024 08:03

Jcnessss requested a review from a team as a code owner October 18, 2024 08:03

Jcnessss force-pushed the map_agg branch from b4a39fd to 944c0d6 Compare October 18, 2024 08:36

Jcnessss requested a review from stdpain October 18, 2024 08:38

stdpain force-pushed the map_agg branch from 944c0d6 to 92200b1 Compare October 22, 2024 06:10

Jcnessss force-pushed the map_agg branch 5 times, most recently from 8879316 to 82f913f Compare October 25, 2024 07:30

Jcnessss force-pushed the map_agg branch 3 times, most recently from d6cd77d to dde5d83 Compare October 28, 2024 02:16

Jcnessss force-pushed the map_agg branch 2 times, most recently from c1a7ef4 to 19cab15 Compare October 30, 2024 02:12

stdpain reviewed Oct 30, 2024

View reviewed changes

Jcnessss force-pushed the map_agg branch from 19cab15 to 3f957f7 Compare October 31, 2024 02:18

[Enhancement] Support aggregate function map_agg.

adf01f0

Signed-off-by: Song Jiacheng <[email protected]>

Jcnessss force-pushed the map_agg branch from 3f957f7 to adf01f0 Compare October 31, 2024 02:55

Jcnessss requested a review from stdpain October 31, 2024 05:53

stdpain approved these changes Nov 1, 2024

View reviewed changes

stdpain enabled auto-merge (squash) November 7, 2024 13:27

kangkaisen approved these changes Nov 7, 2024

View reviewed changes

Seaven approved these changes Nov 8, 2024

View reviewed changes

gengjun-git approved these changes Nov 8, 2024

View reviewed changes

stdpain merged commit b8447fb into StarRocks:main Nov 8, 2024
53 of 54 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Enhancement] Support aggregate function map_agg. #51967

[Enhancement] Support aggregate function map_agg. #51967

Jcnessss commented Oct 16, 2024 •

edited

Loading

stdpain Oct 16, 2024

Jcnessss Oct 16, 2024

Jcnessss commented Oct 16, 2024

Jcnessss commented Oct 17, 2024

Jcnessss commented Oct 18, 2024

stdpain commented Oct 22, 2024

mergify bot commented Oct 22, 2024

stdpain commented Oct 22, 2024

mergify bot commented Oct 22, 2024

Jcnessss commented Oct 28, 2024 •

edited

Loading

stdpain Oct 30, 2024 •

edited

Loading

Jcnessss Oct 31, 2024

sonarcloud bot commented Oct 31, 2024

github-actions bot commented Oct 31, 2024

github-actions bot commented Oct 31, 2024

github-actions bot commented Oct 31, 2024

Jcnessss commented Nov 8, 2024

-        }
+        }
+        if (dst->is_nullable()) {
+            down_cast<NullableColumn*>(dst)->null_column_data().resize(dst->size() + chunk_size);
+        }

[Enhancement] Support aggregate function map_agg. #51967

[Enhancement] Support aggregate function map_agg. #51967

Conversation

Jcnessss commented Oct 16, 2024 • edited Loading

Why I'm doing:

What I'm doing:

What type of PR is this:

Checklist:

Bugfix cherry-pick branch check:

stdpain Oct 16, 2024

Choose a reason for hiding this comment

Jcnessss Oct 16, 2024

Choose a reason for hiding this comment

Jcnessss commented Oct 16, 2024

Jcnessss commented Oct 17, 2024

Jcnessss commented Oct 18, 2024

stdpain commented Oct 22, 2024

mergify bot commented Oct 22, 2024

❌ Unable to rebase: user stdpain is unknown.

stdpain commented Oct 22, 2024

mergify bot commented Oct 22, 2024

✅ Branch has been successfully rebased

Jcnessss commented Oct 28, 2024 • edited Loading

stdpain Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Jcnessss Oct 31, 2024

Choose a reason for hiding this comment

sonarcloud bot commented Oct 31, 2024

Quality Gate passed

github-actions bot commented Oct 31, 2024

[Java-Extensions Incremental Coverage Report]

github-actions bot commented Oct 31, 2024

[FE Incremental Coverage Report]

file detail

github-actions bot commented Oct 31, 2024

[BE Incremental Coverage Report]

file detail

Jcnessss commented Nov 8, 2024

Jcnessss commented Oct 16, 2024 •

edited

Loading

❌ Unable to rebase: user `stdpain` is unknown.

Jcnessss commented Oct 28, 2024 •

edited

Loading

stdpain Oct 30, 2024 •

edited

Loading