Collect plan statistics for explain only by default #1866

sopel39 · 2019-10-25T14:03:38Z

Plan stats collection creates considerable load on
Hive metastore (even when CBO is disabled). Therefore
is better not to collect stats for plan for
non-explain queries

presto-main/src/main/java/io/prestosql/sql/analyzer/FeaturesConfig.java

sopel39 · 2019-11-01T21:59:47Z

@findepi AC

presto-main/src/main/java/io/prestosql/SystemSessionProperties.java

presto-main/src/main/java/io/prestosql/sql/analyzer/FeaturesConfig.java

presto-main/src/main/java/io/prestosql/sql/analyzer/QueryExplainer.java

presto-main/src/main/java/io/prestosql/sql/analyzer/FeaturesConfig.java

presto-main/src/main/java/io/prestosql/sql/planner/LogicalPlanner.java

presto-tests/src/test/java/io/prestosql/tests/TestTpchDistributedStats.java

presto-tests/src/main/java/io/prestosql/tests/statistics/StatisticsAssertion.java

sopel39

AC @findepi

presto-main/src/main/java/io/prestosql/SystemSessionProperties.java

presto-main/src/main/java/io/prestosql/sql/analyzer/QueryExplainer.java

presto-main/src/main/java/io/prestosql/sql/planner/LogicalPlanner.java

sopel39 · 2019-11-04T10:53:08Z

presto-main/src/main/java/io/prestosql/sql/planner/LogicalPlanner.java

+        StatsAndCosts statsAndCosts = StatsAndCosts.empty();
+        if (collectStatsAndCosts) {
+            statsAndCosts = StatsAndCosts.create(root, statsProvider, costProvider);
+        }


It's more lines this way

sopel39 · 2019-11-04T10:54:08Z

presto-main/src/main/java/io/prestosql/testing/LocalQueryRunner.java

@@ -848,7 +848,7 @@ public Plan createPlan(Session session, @Language("SQL") String sql, List<PlanOp
        LogicalPlanner logicalPlanner = new LogicalPlanner(session, optimizers, new PlanSanityChecker(true), idAllocator, metadata, new TypeAnalyzer(sqlParser, metadata), statsCalculator, costCalculator, warningCollector);

        Analysis analysis = analyzer.analyze(preparedQuery.getStatement());
-        return logicalPlanner.plan(analysis, stage);
+        return logicalPlanner.plan(analysis, stage, true);


LocalQueryRunner is mostly used in tests, therefore it's OK (and actually simpler) to have it collect plan stats always.

presto-main/src/main/java/io/prestosql/SystemSessionProperties.java

findepi · 2019-11-04T12:01:21Z

presto-main/src/main/java/io/prestosql/sql/planner/LogicalPlanner.java

@@ -162,6 +163,11 @@ public Plan plan(Analysis analysis)
    }

    public Plan plan(Analysis analysis, Stage stage)
+    {
+        return plan(analysis, stage, analysis.getStatement() instanceof Explain || isCollectPlanStatisticsForAllQueries(session));


Since QueryExplainer passes true here, do we need to have instanceof?
i don't like instanceof.

if this is about EXPLAIN ANALYZE, we should be able to pass this information in some better way.

there is already similar check io/prestosql/execution/SqlQueryExecution.java:410

if this is about EXPLAIN ANALYZE, we should be able to pass this information in some better way.

Maybe there is some other way, but is it worth to trade it for such simple change?

is this check here for EXPLAIN or EXPLAIN ANALYZE?

is this check here for EXPLAIN or EXPLAIN ANALYZE?

EXPLAIN, (any kind of).

findepi · 2019-11-04T12:02:37Z

presto-tests/src/test/java/io/prestosql/tests/TestTpchDistributedStats.java

+                        // We are not able to calculate stats for PARTIAL aggregations
+                        .setSystemProperty(PREFER_PARTIAL_AGGREGATION, "false")
+                        // Distributed query runner does not collect stats for non-EXPLAIN queries by default
+                        .setSystemProperty(COLLECT_PLAN_STATISTICS_FOR_ALL_QUERIES, "true"))


Thanks for the comment. I still wonder why we have to set it here, in this class.

because we need them for stats tests, which the class name suggest?

which the class name suggest?

as it is (// Distributed query runner does not collect stats for non-EXPLAIN queries by default) it just states what the default is, no point in saying it here

i hoped for something more direct, but let it assume it's self-explanatory

presto-tests/src/main/java/io/prestosql/tests/statistics/MetricComparison.java

Plan stats collection creates considerable load on Hive metastore (even when CBO is disabled). Therefore is better not to collect stats for plan for non-explain queries

cla-bot bot added the cla-signed label Oct 25, 2019

findepi reviewed Oct 25, 2019

View reviewed changes

presto-main/src/main/java/io/prestosql/sql/analyzer/FeaturesConfig.java Outdated Show resolved Hide resolved

sopel39 force-pushed the ks/stats_collection branch from 38c16a4 to d5bfca2 Compare November 1, 2019 21:59

sopel39 force-pushed the ks/stats_collection branch from 699d1ab to 4184ca2 Compare November 2, 2019 00:50

findepi reviewed Nov 4, 2019

View reviewed changes

sopel39 force-pushed the ks/stats_collection branch from 4184ca2 to 7055a31 Compare November 4, 2019 11:07

sopel39 commented Nov 4, 2019

View reviewed changes

sopel39 force-pushed the ks/stats_collection branch from 7055a31 to 19c657e Compare November 4, 2019 12:03

findepi reviewed Nov 4, 2019

View reviewed changes

sopel39 force-pushed the ks/stats_collection branch 2 times, most recently from 320833c to 3b0e30b Compare November 4, 2019 13:05

findepi approved these changes Nov 4, 2019

View reviewed changes

sopel39 added 4 commits November 4, 2019 15:11

By default collect plan statistics for EXPLAIN queries only

4379666

Plan stats collection creates considerable load on Hive metastore (even when CBO is disabled). Therefore is better not to collect stats for plan for non-explain queries

Static import LogicalPlanner.Stage enums

005ef03

Remove MetricComparison.Result#NO_BASELINE

169f5cb

Remove unused method

8971d84

sopel39 force-pushed the ks/stats_collection branch from 3b0e30b to 8971d84 Compare November 4, 2019 14:11

sopel39 merged commit e74129d into trinodb:master Nov 4, 2019

sopel39 mentioned this pull request Nov 4, 2019

Release notes for 325 #1931

Closed

6 tasks

martint added this to the 325 milestone Nov 14, 2019

sopel39 deleted the ks/stats_collection branch December 4, 2019 09:55

sopel39 mentioned this pull request Jan 9, 2020

Add 2019 summary post trinodb/trino.io#66

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collect plan statistics for explain only by default #1866

Collect plan statistics for explain only by default #1866

sopel39 commented Oct 25, 2019

sopel39 commented Nov 1, 2019

sopel39 left a comment

sopel39 Nov 4, 2019

sopel39 Nov 4, 2019

findepi Nov 4, 2019

sopel39 Nov 4, 2019 •

edited

findepi Nov 4, 2019

sopel39 Nov 4, 2019

findepi Nov 4, 2019

sopel39 Nov 4, 2019 •

edited

findepi Nov 4, 2019

Collect plan statistics for explain only by default #1866

Collect plan statistics for explain only by default #1866

Conversation

sopel39 commented Oct 25, 2019

sopel39 commented Nov 1, 2019

sopel39 left a comment

Choose a reason for hiding this comment

sopel39 Nov 4, 2019

Choose a reason for hiding this comment

sopel39 Nov 4, 2019

Choose a reason for hiding this comment

findepi Nov 4, 2019

Choose a reason for hiding this comment

sopel39 Nov 4, 2019 • edited

Choose a reason for hiding this comment

findepi Nov 4, 2019

Choose a reason for hiding this comment

sopel39 Nov 4, 2019

Choose a reason for hiding this comment

findepi Nov 4, 2019

Choose a reason for hiding this comment

sopel39 Nov 4, 2019 • edited

Choose a reason for hiding this comment

findepi Nov 4, 2019

Choose a reason for hiding this comment

sopel39 Nov 4, 2019 •

edited

sopel39 Nov 4, 2019 •

edited