[native] Add ANALYZE STATS support by karteekmurthys · Pull Request #20055 · prestodb/presto

karteekmurthys · 2023-07-05T22:56:24Z

Analyze stats requires functions max_data_size_for_stats and sum_data_size_for_stats that were implemented in Velox. But these are internal functions in Presto mapped to presto.default.$internal$ namesapce.

Translate the name mappings correctly during fragment translation to Velox to support "ANALYZE STATS"

Resolves: facebookincubator/velox#5447 and facebookincubator/velox#5484

aditi-pandit · 2023-07-06T00:50:15Z

@karteekmurthys : Do you have an e2e test ?

karteekmurthys · 2023-07-06T00:55:14Z

@karteekmurthys : Do you have an e2e test ?

I will merge analyze tests with this PR. We cannot directly call internal functions as a user.

aditi-pandit · 2023-07-06T01:07:55Z

@karteekmurthys : Do you have an e2e test ?

I will merge analyze tests with this PR. We cannot directly call internal functions as a user.

Yes, please add the analyze tests.

aditi-pandit · 2023-07-08T19:08:44Z

...e-execution/src/test/java/com/facebook/presto/spark/TestPrestoSparkNativeGeneralQueries.java

Nit : Please add a newline between the tests.

aditi-pandit · 2023-07-08T19:09:47Z

...ecution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeGeneralQueries.java

Nit : Please add a newline here.

aditi-pandit · 2023-07-08T19:10:27Z

...ecution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeGeneralQueries.java

This comment can be removed I think. Only please add a comment that the query results in a table giving the (column_name, data_size, distinct_values_count, nulls_fraction, row_count, low_value, high_value).

...ecution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeGeneralQueries.java

aditi-pandit · 2023-07-08T19:11:48Z

...ecution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeGeneralQueries.java

This comment is not needed.

mbasmanova · 2023-07-11T21:26:16Z

...e-execution/src/test/java/com/facebook/presto/spark/TestPrestoSparkNativeGeneralQueries.java

Why is this ignored? Would you add a comment and perhaps refer to a GitHub issue that explains the problem.

@aditi-pandit @karteekmurthys Would take a look at this question? I'm still wondering about this.

I have opened an issue that i faced during testing. Please refer L73.

mbasmanova

@karteekmurthys Please, make sure all comments represent a standalone change and have [native] or [native pos] prefix. Make sure there are not commits like "address review comments". Please, start commit titles with active verbs.

mbasmanova · 2023-07-11T21:29:41Z

presto-native-execution/presto_cpp/main/types/PrestoToVeloxExpr.cpp

kAggregateFunctionsMap -> kFunctionNames for readability; 'Aggregate' is already part of the encoding function name.

mbasmanova · 2023-07-11T21:30:18Z

...ecution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeGeneralQueries.java

/* Expected number of rows updated */

Please, remove this comment. Adding such comments to each assertUpdate call will make code unreadable.

mbasmanova · 2023-07-11T21:31:35Z

...ecution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeGeneralQueries.java

What does this do? How do you verify the results of the SHOW STATS command?

CMIW, assertQuery compares the results of presto and prestissimo. Do I need to explicit check for column values?
https://prestodb.io/docs/current/sql/show-stats.html

I'm thinking that you run ANALYZE on Prestissimo and therefore we don't know if it works or not. If it doesn't work, then SHOW STATS will show nothing for both Prestissimo and Presto and the test will pass. That's not what we want, is it?

Also, if ANALYZE works but computes stats incorrectly, the tests won't detect that, will it?

@mbasmanova : We had been thinking about what would be a more complete test here to cover the situations you outlined.

i) One option is to compare the results of SHOW STATS with values expected in a static list. This list is populated with what we consider correct results.
ii) The Java side also does Statement object level tests. We don't have that framework in prestissimo testing.

Right now the only guarantee is that the sequence of operations on either side are consistent.

But i) would make it more complete.

Wdyt ?

@mbasmanova Aditi is talking about something like this: https://github.com/prestodb/presto/blob/master/presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java#L4426

i) One option is to compare the results of SHOW STATS with values expected in a static list. This list is populated with what we consider correct results.

Sounds reasonable to me. Thanks.

mbasmanova

@karteekmurthys Please, review the list of commits and (1) decide if you need 4 commits or 1 commit would be sufficient; (2) Add [native] or [native pos] prefix to all commit titles; (3) check the changes in each commit to make sure they match the commit message.

karteekmurthys · 2023-07-14T19:30:38Z

@karteekmurthys Please, review the list of commits and (1) decide if you need 4 commits or 1 commit would be sufficient; (2) Add [native] or [native pos] prefix to all commit titles; (3) check the changes in each commit to make sure they match the commit message.

I have squashed it down to single commit now. Please review.

karteekmurthys · 2023-07-17T19:50:20Z

@mbasmanova would you please review this.

mbasmanova

@karteekmurthys Looks good to me, but CI is red.

Curious, why this doesn't work for PoS? CC: @vermapratyush

mbasmanova

@karteekmurthys It would be nice to update commit message to include more details about this change.

karteekmurthys requested a review from a team as a code owner July 5, 2023 22:56

karteekmurthys marked this pull request as draft July 5, 2023 22:56

karteekmurthys self-assigned this Jul 5, 2023

karteekmurthys requested a review from aditi-pandit July 6, 2023 05:04

karteekmurthys force-pushed the map-internal-functions branch 2 times, most recently from d6df1fd to d2ad340 Compare July 6, 2023 22:16

karteekmurthys mentioned this pull request Jul 7, 2023

Add native analyze e2e tests #20028

Closed

karteekmurthys marked this pull request as ready for review July 7, 2023 04:27

aditi-pandit reviewed Jul 8, 2023

View reviewed changes

aditi-pandit approved these changes Jul 10, 2023

View reviewed changes

aditi-pandit requested a review from a team July 11, 2023 21:23

mbasmanova reviewed Jul 11, 2023

View reviewed changes

mbasmanova requested changes Jul 11, 2023

View reviewed changes

mbasmanova reviewed Jul 11, 2023

View reviewed changes

mbasmanova changed the title ~~Map presto internal function names to velox function names~~ [native] Map internal aggregate function names Jul 11, 2023

karteekmurthys mentioned this pull request Jul 13, 2023

[Native] Spark e2e tests for Analyze failure #20294

Closed

karteekmurthys force-pushed the map-internal-functions branch 3 times, most recently from 7aab88c to 4d17b1f Compare July 13, 2023 18:38

aditi-pandit requested a review from mbasmanova July 13, 2023 23:14

mbasmanova requested changes Jul 14, 2023

View reviewed changes

[native] Map internal aggregate function names

24879ed

karteekmurthys force-pushed the map-internal-functions branch from 4d17b1f to 24879ed Compare July 14, 2023 17:50

karteekmurthys closed this Jul 14, 2023

karteekmurthys reopened this Jul 14, 2023

aditi-pandit requested a review from mbasmanova July 18, 2023 20:23

mbasmanova reviewed Jul 19, 2023

View reviewed changes

mbasmanova approved these changes Jul 19, 2023

View reviewed changes

karteekmurthys changed the title ~~[native] Map internal aggregate function names~~ [native] Map internal aggregate function names and add analyze stats e2e tests Jul 19, 2023

aditi-pandit changed the title ~~[native] Map internal aggregate function names and add analyze stats e2e tests~~ [native] Add analyze stats support Jul 19, 2023

aditi-pandit changed the title ~~[native] Add analyze stats support~~ [native] Add ANALYZE STATS support Jul 19, 2023

karteekmurthys merged commit e1b6f76 into prestodb:master Jul 20, 2023

wanglinsong mentioned this pull request Jul 27, 2023

Add release notes for 0.283 #20402

Merged

28 tasks

Conversation

karteekmurthys commented Jul 5, 2023 • edited by aditi-pandit Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aditi-pandit commented Jul 6, 2023

Uh oh!

karteekmurthys commented Jul 6, 2023

Uh oh!

aditi-pandit commented Jul 6, 2023

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!

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!

mbasmanova left a comment

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!

aditi-pandit Jul 12, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

karteekmurthys commented Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karteekmurthys commented Jul 17, 2023

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

karteekmurthys commented Jul 5, 2023 •

edited by aditi-pandit

Loading

aditi-pandit Jul 12, 2023 •

edited

Loading

karteekmurthys commented Jul 14, 2023 •

edited

Loading