Fix Spark first/last aggregate functions intermediate type from T to row(T, boolean) #4873

Yohahaha · 2023-05-09T07:03:19Z

Fix first/last Spark aggregate functions' intermediate type from T to row(T, boolean) to align with Spark, the second column in signature is just a place holder.

oap-project#245
apache/incubator-gluten#1581

netlify · 2023-05-09T07:03:25Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`5c574db`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/64a65f34c2e9dd00085fa87b

Yohahaha · 2023-05-09T08:51:57Z

Hi @mbasmanova could you help take a look?

This PR fix first/last aggregate function's intermediate type from T to row(T, boolean), second column just a place holder.

mbasmanova

@Yuhta @kagamiori Jimmy, Wei, would you help review this PR?

Yohahaha · 2023-05-11T14:46:35Z

Hi @Yuhta @kagamiori could you help review this PR？

Yohahaha · 2023-05-17T08:09:16Z

Hi @mbasmanova @Yuhta @kagamiori could you help take a look?

Yohahaha · 2023-06-05T13:27:33Z

Hi @mbasmanova @Yuhta @kagamiori would you help take a look on this PR? Thanks!

Yohahaha · 2023-06-12T09:31:16Z

This PR fix first/last aggregate function's intermediate type from T to row(T, boolean), and add decimal support.

Could someone help take a look? thanks! @mbasmanova @kagamiori @Yuhta @kgpai

Yohahaha · 2023-06-29T07:02:31Z

Hi @mbasmanova @Yuhta @majetideepak @kagamiori, this pr has already merged in oap/velox and tested by oap/gluten, could someone help take a look?

mbasmanova

Some comments.

velox/functions/sparksql/aggregates/FirstLastAggregate.cpp

Yohahaha · 2023-06-30T09:09:26Z

Updates:

Separate decimal support to another PR, update PR title and description.
Remove dynamic_cast.
Use DecodedVector::isNullAt.

@mbasmanova could you review again? thanks!

mbasmanova

@Yohahaha Thank you for iterating on this PR. Some further comments.

velox/functions/sparksql/aggregates/FirstLastAggregate.cpp

Yohahaha · 2023-07-06T02:10:49Z

Updates:

Remove exec::
Minor refactor for updateValue to avoid duplicate casting
Minor refactor for reuse DecodedVector

@mbasmanova could you help review again? thanks!

mbasmanova

Looks good % one question.

velox/functions/sparksql/aggregates/FirstLastAggregate.cpp

facebook-github-bot · 2023-07-06T03:00:04Z

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

mbasmanova · 2023-07-06T03:44:01Z

Would you rebase on top of latest main so I could merge?

fix Trigger CI fix format

Yohahaha · 2023-07-06T03:55:43Z

Would you rebase on top of latest main so I could merge?

Sure, already rebased, thanks!

facebook-github-bot · 2023-07-06T04:27:19Z

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

mbasmanova · 2023-07-06T04:56:50Z

Looks like Spark Fuzzer failed. Please, take a look.

Yohahaha · 2023-07-06T04:59:36Z

Looks like Spark Fuzzer failed. Please, take a look.

Thanks! I will fix it and run a long fuzzer.

Yohahaha · 2023-07-06T07:02:06Z

./spark_aggregation_fuzzer_test --seed 2677297971 --duration_sec 1800 --logtostderr=1 --minloglevel=0 --repro_persist_path=/tmp/spark_aggregate_fuzzer_repro

==============================> Done with iteration 4645
 Total functions tested: 5
 Total masked aggregations: 744 (16.01%)
 Total global aggregations: 388 (8.35%)
 Total group-by aggregations: 3391 (72.99%)
 Total distinct aggregations: 452 (9.73%)
 Total window expressions: 415 (8.93%)
 Total aggregations verified against DuckDB: 1747 (37.60%)
 Total failed aggregations: 0 (0.00%)

Yohahaha · 2023-07-06T07:04:48Z

The root cause was I passed decoded.index(i) to updateValue in addRawInputs, it's lead index overflow error.

facebook-github-bot · 2023-07-06T13:20:58Z

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-07-06T16:57:58Z

@mbasmanova merged this pull request in 9fdf36e.

conbench-facebook · 2023-07-06T17:16:44Z

Conbench analyzed the 1 benchmark run on commit 9fdf36ed.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 9, 2023

Yohahaha changed the title ~~Fix Spark First/Last aggregate functions intermediate signature~~ Fix Spark First/Last aggregate functions intermediate type May 9, 2023

mbasmanova reviewed May 9, 2023

View reviewed changes

Yohahaha marked this pull request as draft May 10, 2023 02:26

Yohahaha marked this pull request as ready for review May 10, 2023 16:28

Yohahaha marked this pull request as draft May 10, 2023 16:29

Yohahaha force-pushed the first_last_meta branch from 7e39e99 to bd3e21f Compare May 11, 2023 07:56

Yohahaha changed the title ~~Fix Spark First/Last aggregate functions intermediate type~~ Fix Spark First/Last aggregate functions intermediate type and support decimal May 11, 2023

Yohahaha marked this pull request as ready for review May 11, 2023 07:59

Yohahaha force-pushed the first_last_meta branch from d97f8d7 to 406044f Compare June 12, 2023 06:18

Yohahaha changed the title ~~Fix Spark First/Last aggregate functions intermediate type and support decimal~~ Fix Spark First/Last aggregate functions intermediate type and add decimal type support Jun 28, 2023

Yohahaha force-pushed the first_last_meta branch from 406044f to 40ea902 Compare June 29, 2023 06:54

mbasmanova requested a review from majetideepak June 29, 2023 07:42

mbasmanova reviewed Jun 29, 2023

View reviewed changes

Yohahaha force-pushed the first_last_meta branch 2 times, most recently from dbf4a6d to c24f24b Compare June 30, 2023 06:46

Yohahaha changed the title ~~Fix Spark First/Last aggregate functions intermediate type and add decimal type support~~ Fix Spark first/last aggregate functions intermediate type from T to row(T, boolean) Jun 30, 2023

mbasmanova reviewed Jun 30, 2023

View reviewed changes

mbasmanova reviewed Jul 6, 2023

View reviewed changes

velox/functions/sparksql/aggregates/FirstLastAggregate.cpp Outdated Show resolved Hide resolved

velox/functions/sparksql/aggregates/FirstLastAggregate.cpp Show resolved Hide resolved

mbasmanova approved these changes Jul 6, 2023

View reviewed changes

fix intermediate type

a52828a

fix Trigger CI fix format

Yohahaha force-pushed the first_last_meta branch from 8e7e45b to a52828a Compare July 6, 2023 03:55

fix

5c574db

facebook-github-bot closed this in 9fdf36e Jul 6, 2023

facebook-github-bot added the Merged label Jul 6, 2023

Yohahaha deleted the first_last_meta branch April 30, 2024 02:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Spark first/last aggregate functions intermediate type from T to row(T, boolean) #4873

Fix Spark first/last aggregate functions intermediate type from T to row(T, boolean) #4873

Yohahaha commented May 9, 2023 •

edited

Loading

netlify bot commented May 9, 2023 •

edited

Loading

Yohahaha commented May 9, 2023 •

edited

Loading

mbasmanova left a comment

Yohahaha commented May 11, 2023 •

edited

Loading

Yohahaha commented May 17, 2023

Yohahaha commented Jun 5, 2023

Yohahaha commented Jun 12, 2023

Yohahaha commented Jun 29, 2023

mbasmanova left a comment

Yohahaha commented Jun 30, 2023

mbasmanova left a comment

Yohahaha commented Jul 6, 2023

mbasmanova left a comment

facebook-github-bot commented Jul 6, 2023

mbasmanova commented Jul 6, 2023

Yohahaha commented Jul 6, 2023

facebook-github-bot commented Jul 6, 2023

mbasmanova commented Jul 6, 2023

Yohahaha commented Jul 6, 2023

Yohahaha commented Jul 6, 2023

Yohahaha commented Jul 6, 2023 •

edited

Loading

facebook-github-bot commented Jul 6, 2023

facebook-github-bot commented Jul 6, 2023

conbench-facebook bot commented Jul 6, 2023

Fix Spark first/last aggregate functions intermediate type from T to row(T, boolean) #4873

Fix Spark first/last aggregate functions intermediate type from T to row(T, boolean) #4873

Conversation

Yohahaha commented May 9, 2023 • edited Loading

netlify bot commented May 9, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

Yohahaha commented May 9, 2023 • edited Loading

mbasmanova left a comment

Choose a reason for hiding this comment

Yohahaha commented May 11, 2023 • edited Loading

Yohahaha commented May 17, 2023

Yohahaha commented Jun 5, 2023

Yohahaha commented Jun 12, 2023

Yohahaha commented Jun 29, 2023

mbasmanova left a comment

Choose a reason for hiding this comment

Yohahaha commented Jun 30, 2023

mbasmanova left a comment

Choose a reason for hiding this comment

Yohahaha commented Jul 6, 2023

mbasmanova left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 6, 2023

mbasmanova commented Jul 6, 2023

Yohahaha commented Jul 6, 2023

facebook-github-bot commented Jul 6, 2023

mbasmanova commented Jul 6, 2023

Yohahaha commented Jul 6, 2023

Yohahaha commented Jul 6, 2023

Yohahaha commented Jul 6, 2023 • edited Loading

facebook-github-bot commented Jul 6, 2023

facebook-github-bot commented Jul 6, 2023

conbench-facebook bot commented Jul 6, 2023

Yohahaha commented May 9, 2023 •

edited

Loading

netlify bot commented May 9, 2023 •

edited

Loading

Yohahaha commented May 9, 2023 •

edited

Loading

Yohahaha commented May 11, 2023 •

edited

Loading

Yohahaha commented Jul 6, 2023 •

edited

Loading