[GLUTEN-1648][VL] Add max_by/min_by aggregate function support #2336

Yohahaha · 2023-07-14T02:32:00Z

No description provided.

github-actions · 2023-07-14T02:32:18Z

Run Gluten Clickhouse CI

github-actions · 2023-07-14T02:34:03Z

Yohahaha · 2023-07-14T02:47:42Z

Spark's max_by has a little different with Presto/Velox in implementation, detailed discussion here facebookincubator/velox#4971 .

Seems Spark UT does not specify these corner cases, let's add these functions for now, and will be keeping modify above PR if it is necessary.

github-actions · 2023-07-14T04:22:34Z

Run Gluten Clickhouse CI

Yohahaha · 2023-07-14T07:09:09Z

@rui-mo @PHILO-HE could you help take a look?

PHILO-HE · 2023-07-14T07:43:25Z

Hi @Yohahaha, thanks for your patch! Recently, to relieve the burden of velox code rebase, we prefer directly contributing velox code to its upstream (exceptions are some fixes on the code only exists in oap/velox, etc.). Could you please try to get the velox patch merged to its upstream?
It's OK to submit this Gluten PR for early code review & Gluten CI verifications. I will add pending velox rebase tag. Thanks!

rui-mo · 2023-07-14T08:00:14Z

Spark's max_by has a little different with Presto/Velox in implementation, detailed discussion here facebookincubator/velox#4971 .

Seems Spark UT does not specify these corner cases, let's add these functions for now, and will be keeping modify above PR if it is necessary.

Hi @Yohahaha, how large would the difference affect the computing? There could be some risks if the functions are offloaded to Velox but with incorrect result.

Yohahaha · 2023-07-14T08:09:35Z

Hi @Yohahaha, thanks for your patch! Recently, to relieve the burden of velox code rebase, we prefer directly contributing velox code to its upstream (exceptions are some fixes on the code only exists in oap/velox, etc.). Could you please try to get the velox patch merged to its upstream? It's OK to submit this Gluten PR for early code review & Gluten CI verifications. I will add pending velox rebase tag. Thanks!

This PR does not include codes which need push to upstream, please check related oap/velox link.

Yohahaha · 2023-07-14T08:14:58Z

Spark's max_by has a little different with Presto/Velox in implementation, detailed discussion here facebookincubator/velox#4971 .
Seems Spark UT does not specify these corner cases, let's add these functions for now, and will be keeping modify above PR if it is necessary.

Hi @Yohahaha, how large would the difference affect the computing? There could be some risks if the functions are offloaded to Velox but with incorrect result.

It's a minor change, different behaviors see below

SELECT max_by(x, y) FROM VALUES ('a', 10), ('b', 50), ('c', 50) AS tab(x, y)

Spark result is c
Presto result is b

Spark does not specify max_by/min_by as deterministic, so I pending update above PR.

rui-mo · 2023-07-14T09:03:22Z

SELECT max_by(x, y) FROM VALUES ('a', 10), ('b', 50), ('c', 50) AS tab(x, y)

Spark result is c
Presto result is b

@Yohahaha Thank you, I see. So the results can be different when there are equal values, right?

Spark does not specify max_by/min_by as deterministic, so I pending update above PR.

Could you explain this statement more a bit? Does that mean Spark's behavior is random?

Yohahaha · 2023-07-14T09:16:18Z

So the results can be different when there are equal values, right?

Yes.

Could you explain this statement more a bit? Does that mean Spark's behavior is random?

If we have 2 equal values and both are largest of group, Spark will return second, however Presto return first. Since input data in agg is non-deterministic, Velox community think there is no need to distinguish that difference.

And I found Spark does not specify these corner cases as well or any comments for that.

rui-mo · 2023-07-17T00:42:52Z

@Yohahaha Could you help open an issue for this corner case in Gluten? Please also update this limitation in https://github.com/oap-project/gluten/blob/main/docs/velox-backend-limitations.md. Thanks.

Yohahaha · 2023-07-17T03:11:13Z

Convert to draft and wait align behavior with Spark.

PHILO-HE · 2023-07-19T02:51:46Z

Please note we have moved the code related to substrait from oap/velox into gluten. So for your proposed changes in oap/velox, please apply them in gluten/cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc. Thanks!

Yohahaha · 2023-08-02T03:27:33Z

Wait oap/velox rebase 8.2

PHILO-HE · 2023-10-19T05:59:31Z

Hi @Yohahaha, as oap/velox is aligned with the upstream, this PR can be merged. Could you please do a rebase?

Yohahaha · 2023-10-20T01:42:19Z

@rui-mo I see this change and may_by/min_by support also need change here, this is your expected?

rui-mo · 2023-10-20T05:55:31Z

@rui-mo I see this change and may_by/min_by support also need change here, this is your expected?

@Yohahaha This change is to allow companion function registration through config. What do you need for max_by and min_by?

Yohahaha · 2023-10-20T06:09:53Z

@rui-mo I see this change and may_by/min_by support also need change here, this is your expected?

@Yohahaha This change is to allow companion function registration through config. What do you need for max_by and min_by?

Spark's min_by/max_by need overwrite presto's, but current register function does not provide these two bool args. I'm not sure whether oap-velox still accept new patches.

https://github.com/oap-project/velox/blob/b8fc63ec35f049a49e417ba4b0845c2b1e367748/velox/functions/sparksql/aggregates/Register.cpp#L33C33-L43

rui-mo · 2023-10-20T06:14:39Z

@Yohahaha This is my draft PR facebookincubator/velox#7110. I can add these two bool args to min_by and max_by if needed.

Yohahaha · 2023-10-20T06:21:51Z

@Yohahaha This is my draft PR facebookincubator/velox#7110. I can add these two bool args to min_by and max_by if needed.

That's great, but I think we could pass specific prefix when register functions such as presto_ or spark_, and add prefix conversion logic in somewhere when convert substait plan to velox plan, it will bring benefits for use more accurate functions. What's your thoughts?

Yohahaha · 2023-10-20T09:30:05Z

#3472

github-actions · 2023-10-20T09:30:41Z

Run Gluten Clickhouse CI

rui-mo · 2023-10-25T02:27:05Z

We've added the overwrite config for max_by and mix_by at Register.cpp#L41. Maybe this PR can be firstly enabled with that. cc @PHILO-HE

Yohahaha · 2023-10-25T02:35:42Z

We've added the overwrite config for max_by and mix_by at Register.cpp#L41. Maybe this PR can be firstly enabled with that. cc @PHILO-HE

Thanks, I will try later.

Yohahaha · 2023-10-27T09:23:17Z

We've added the overwrite config for max_by and mix_by at Register.cpp#L41. Maybe this PR can be firstly enabled with that. cc @PHILO-HE

depends on oap-project/velox#426

github-actions · 2023-10-27T09:26:31Z

Run Gluten Clickhouse CI

rui-mo · 2023-10-30T00:50:46Z

@Yohahaha There are code style failures. Please refer to https://github.com/oap-project/gluten/blob/main/docs/developers/NewToGluten.md#javascala-code-style for code style adjustment.

github-actions · 2023-10-30T02:20:08Z

Run Gluten Clickhouse CI

github-actions · 2023-11-06T02:17:57Z

Run Gluten Clickhouse CI

github-actions · 2023-11-06T06:26:30Z

Run Gluten Clickhouse CI

Yohahaha · 2023-11-06T09:33:31Z

CI passed.

Yohahaha · 2023-11-07T02:10:45Z

please help review again, thanks! @rui-mo @PHILO-HE

PHILO-HE

Thanks!

rui-mo · 2023-11-07T02:49:28Z

@Yohahaha Could you resolve the conflicts? Thanks.

github-actions · 2023-11-08T03:19:37Z

Run Gluten Clickhouse CI

github-actions · 2023-11-09T01:46:12Z

Run Gluten Clickhouse CI

Yohahaha · 2023-11-09T08:20:39Z

could we merge?

PHILO-HE · 2023-11-09T08:25:19Z

backends-clickhouse/src/main/scala/io/glutenproject/utils/CHExpressionUtil.scala

@@ -153,6 +153,8 @@ case class EncodeDecodeValidator() extends FunctionValidator {
 object CHExpressionUtil {

  final val CH_AGGREGATE_FUNC_BLACKLIST: Map[String, FunctionValidator] = Map(
+    MAX_BY -> DefaultValidator(),


GlutenPerfBot · 2023-11-09T11:49:31Z

===== Performance report for TPCH SF2000 with Velox backend, for reference only ====

query	log/native_2336_time.csv	log/native_master_11_08_2023_29b58997a_time.csv	difference	percentage
q1	34.21	34.11	-0.094	99.73%
q2	25.55	25.13	-0.417	98.37%
q3	36.75	38.62	1.867	105.08%
q4	38.28	36.07	-2.205	94.24%
q5	71.63	71.84	0.214	100.30%
q6	7.96	7.59	-0.372	95.33%
q7	84.50	85.82	1.317	101.56%
q8	87.28	86.97	-0.312	99.64%
q9	123.47	121.86	-1.610	98.70%
q10	52.42	54.80	2.381	104.54%
q11	20.13	20.59	0.456	102.26%
q12	25.23	24.63	-0.596	97.64%
q13	49.14	49.95	0.819	101.67%
q14	17.55	16.99	-0.560	96.81%
q15	30.29	30.97	0.677	102.23%
q16	15.88	16.51	0.630	103.97%
q17	101.25	102.40	1.151	101.14%
q18	149.37	148.28	-1.095	99.27%
q19	14.95	15.13	0.188	101.25%
q20	29.13	29.97	0.838	102.88%
q21	226.85	222.20	-4.653	97.95%
q22	13.44	13.55	0.109	100.81%
total	1255.27	1254.01	-1.266	99.90%

Yohahaha mentioned this pull request Jul 14, 2023

support max_by and min_by aggregate function oap-project/velox#363

Closed

Yohahaha changed the title ~~[VL] Add max_by/min_by aggregate function support~~ [GLUTEN-1648][VL] Add max_by/min_by aggregate function support Jul 14, 2023

Yohahaha force-pushed the max_by branch from 981f7c9 to 7c87b34 Compare July 14, 2023 04:22

PHILO-HE added the pending velox rebase the patch will be merged after velox rebased with Meta main branch label Jul 14, 2023

Yohahaha marked this pull request as draft July 17, 2023 03:10

Yohahaha closed this Oct 20, 2023

Yohahaha reopened this Oct 20, 2023

rui-mo mentioned this pull request Oct 23, 2023

[VL] Respect prefix when register functions and convert function name #3472

Open

Yohahaha force-pushed the max_by branch from 7c87b34 to 11e8db0 Compare October 27, 2023 09:26

Yohahaha marked this pull request as ready for review October 27, 2023 09:26

Yohahaha force-pushed the max_by branch from b87ae4d to eb2e645 Compare November 6, 2023 02:17

PHILO-HE previously approved these changes Nov 7, 2023

View reviewed changes

Yohahaha dismissed PHILO-HE’s stale review via 52baa11 November 8, 2023 03:19

Yohahaha force-pushed the max_by branch from f56114c to 52baa11 Compare November 8, 2023 03:19

Yohahaha added 2 commits November 9, 2023 09:44

support max_by and min_by

3a9e565

skip for CH backend.

d82074e

Yohahaha force-pushed the max_by branch from 52baa11 to d82074e Compare November 9, 2023 01:45

rui-mo requested a review from zzcclp November 9, 2023 08:23

PHILO-HE approved these changes Nov 9, 2023

View reviewed changes

PHILO-HE reviewed Nov 9, 2023

View reviewed changes

PHILO-HE merged commit fee6529 into apache:main Nov 9, 2023
17 checks passed

Yohahaha deleted the max_by branch November 10, 2023 01:44

[GLUTEN-1648][VL] Add max_by/min_by aggregate function support #2336

[GLUTEN-1648][VL] Add max_by/min_by aggregate function support #2336

Conversation

Yohahaha commented Jul 14, 2023

github-actions bot commented Jul 14, 2023

github-actions bot commented Jul 14, 2023

Yohahaha commented Jul 14, 2023 • edited Loading

github-actions bot commented Jul 14, 2023

Yohahaha commented Jul 14, 2023

PHILO-HE commented Jul 14, 2023 • edited Loading

rui-mo commented Jul 14, 2023

Yohahaha commented Jul 14, 2023

Yohahaha commented Jul 14, 2023

rui-mo commented Jul 14, 2023 • edited Loading

Yohahaha commented Jul 14, 2023 • edited Loading

rui-mo commented Jul 17, 2023 • edited Loading

Yohahaha commented Jul 17, 2023

PHILO-HE commented Jul 19, 2023

Yohahaha commented Aug 2, 2023

PHILO-HE commented Oct 19, 2023

Yohahaha commented Oct 20, 2023

rui-mo commented Oct 20, 2023

Yohahaha commented Oct 20, 2023 • edited Loading

rui-mo commented Oct 20, 2023

Yohahaha commented Oct 20, 2023

Yohahaha commented Oct 20, 2023

github-actions bot commented Oct 20, 2023

rui-mo commented Oct 25, 2023 • edited Loading

Yohahaha commented Oct 25, 2023

Yohahaha commented Oct 27, 2023

github-actions bot commented Oct 27, 2023

rui-mo commented Oct 30, 2023

github-actions bot commented Oct 30, 2023

github-actions bot commented Nov 6, 2023

github-actions bot commented Nov 6, 2023

Yohahaha commented Nov 6, 2023

Yohahaha commented Nov 7, 2023

PHILO-HE left a comment

Choose a reason for hiding this comment

rui-mo commented Nov 7, 2023

github-actions bot commented Nov 8, 2023

github-actions bot commented Nov 9, 2023

Yohahaha commented Nov 9, 2023

PHILO-HE Nov 9, 2023

Choose a reason for hiding this comment

zzcclp Nov 10, 2023

Choose a reason for hiding this comment

GlutenPerfBot commented Nov 9, 2023

Yohahaha commented Jul 14, 2023 •

edited

Loading

PHILO-HE commented Jul 14, 2023 •

edited

Loading

rui-mo commented Jul 14, 2023 •

edited

Loading

Yohahaha commented Jul 14, 2023 •

edited

Loading

rui-mo commented Jul 17, 2023 •

edited

Loading

Yohahaha commented Oct 20, 2023 •

edited

Loading

rui-mo commented Oct 25, 2023 •

edited

Loading