Include cost estimates in IO EXPLAIN plan #806

JamesRTaylor · 2019-05-22T19:57:07Z

Implementation of #677

presto-main/src/main/java/io/prestosql/sql/planner/planprinter/IoPlanPrinter.java

presto-product-tests/src/main/java/io/prestosql/tests/hive/TestHiveTableStatistics.java

presto-main/src/main/java/io/prestosql/sql/planner/planprinter/IoPlanPrinter.java

ebyhr · 2019-05-23T23:30:25Z

While I already sent PR #709 (please ignore mine, I'll close it), why only estimated byte scanned? I think other information (e.g. outputRowCount) is also useful.

Of course we can limit the field first. However, if you add estBytesScanned to TableColumnInfo directly, the output format will be like below.

{
  "inputTableColumnInfos": [
    {
      "table": {..
      "columns": [ ..
      "estimatedBytesScanned": 1230000
    }
  ],

While it's easy to parse, when we add fields later about estimated costs, it looks little redundant to me.

{
  "inputTableColumnInfos": [
    {
      "table": {
      "columns": [ ..
      "estimatedBytesScanned": 1230000
      "estimatedxxx": 999
      "estimatedyyy": 999
    }
  ],

I hope you update presto-docs/src/main/sphinx/sql/explain.rst so that other developers can use this enhancement easily.

findepi · 2019-05-24T08:19:19Z

While I already sent PR #709 ...

@ebyhr apologies it hasn't been reviewed yet!

@ebyhr @JamesRTaylor can you look at each other's PR and determine how much overlap is there?

ebyhr · 2019-05-24T08:39:10Z

@findepi There's no need to apologize. Actually, I had completely forgot my PR until today and I hope @JamesRTaylor (the issue ticket author) implements this. Also, I'm little busy in this month.

The main difference between my PR and this commit is I added a separated class and 5 metrics including estimated byte scanned. If there's no special reason to limit to the bytes, adding other 4 metrics looks good to me. @JamesRTaylor What do you think about it?

JamesRTaylor · 2019-06-19T21:46:47Z

Thanks so much for your comments, @kokosing, @ebyhr, and @findepi. I've pushed an update to address the feedback. Please review when you get a chance.

JamesRTaylor · 2019-07-08T22:21:05Z

Ping @kokosing, @dain, @martint - please review when you have a few spare cycles. Thanks!

kokosing · 2019-07-09T10:03:58Z

Can you please rebase and get rid of merge commits (we do not use them).

JamesRTaylor · 2019-07-09T19:32:44Z

@kokosing - I rebased and squashed the commits. I looked at the test failure, but it doesn't appear related.

findepi · 2019-07-09T21:24:32Z

@JamesRTaylor from Travis:

[ERROR] TestTpchDistributedQueries.testIOExplain:70 expected [IoPlan{inputTableColumnInfos=[TableColumnInfo{table=tpch.sf0.01.orders, columnConstraints=[ColumnConstraint{columnName=orderstatus, typeSignature=varchar(1), domain=FormattedDomain{nullsAllowed=false, ranges=[FormattedRange{low=FormattedMarker{value=Optional[F], bound=EXACTLY}, high=FormattedMarker{value=Optional[F], bound=EXACTLY}}, FormattedRange{low=FormattedMarker{value=Optional[O], bound=EXACTLY}, high=FormattedMarker{value=Optional[O], bound=EXACTLY}}, FormattedRange{low=FormattedMarker{value=Optional[P], bound=EXACTLY}, high=FormattedMarker{value=Optional[P], bound=EXACTLY}}]}}], estimate=EstimatedStatsAndCost{outputRowCount=15000.0, outputSizeInBytes=1597294.0, cpuCost=1597294.0, maxMemory=0.0, networkCost=0.0, bytesScanned=0.0}}], outputTable=Optional.empty, estimate=EstimatedStatsAndCost{outputRowCount=15000.0, outputSizeInBytes=1597294.0, cpuCost=1597294.0, maxMemory=0.0, networkCost=0.0, bytesScanned=0.0}}] but found [IoPlan{inputTableColumnInfos=[TableColumnInfo{table=tpch.sf0.01.orders, columnConstraints=[ColumnConstraint{columnName=orderstatus, typeSignature=varchar(1), domain=FormattedDomain{nullsAllowed=false, ranges=[FormattedRange{low=FormattedMarker{value=Optional[F], bound=EXACTLY}, high=FormattedMarker{value=Optional[F], bound=EXACTLY}}, FormattedRange{low=FormattedMarker{value=Optional[O], bound=EXACTLY}, high=FormattedMarker{value=Optional[O], bound=EXACTLY}}, FormattedRange{low=FormattedMarker{value=Optional[P], bound=EXACTLY}, high=FormattedMarker{value=Optional[P], bound=EXACTLY}}]}}], estimate=EstimatedStatsAndCost{outputRowCount=15000.0, outputSizeInBytes=1597294.0, cpuCost=1597294.0, maxMemory=0.0, networkCost=0.0, bytesScanned=742294.0}}], outputTable=Optional.empty, estimate=EstimatedStatsAndCost{outputRowCount=15000.0, outputSizeInBytes=1597294.0, cpuCost=1597294.0, maxMemory=0.0, networkCost=1597294.0, bytesScanned=742294.0}}]

JamesRTaylor · 2019-07-09T22:07:09Z

Oops, missed that - sorry about that. Was seeing other errors in the console that I thought was leading to the test failure. I'll get that fixed.

JamesRTaylor · 2019-07-10T00:55:05Z

All fixed now, @findepi. Sorry again for the noise.

presto-docs/src/main/sphinx/sql/explain.rst

kokosing · 2019-07-11T07:40:15Z

presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java

+        EstimatedStatsAndCost estimate16 = new EstimatedStatsAndCost(1.0, 16.0, 16.0, 0.0, 0.0, 6.0);
+        EstimatedStatsAndCost estimate17 = new EstimatedStatsAndCost(1.0, 17.0, 17.0, 0.0, 0.0, 3.0);
+        EstimatedStatsAndCost estimate25 = new EstimatedStatsAndCost(1.0, 25.0, 25.0, 0.0, 0.0, 3.0);
+        Map<Object, TypeAndEstimate> data = new LinkedHashMap<>();


why linked hash map?

It's a linked hash map because the assert below prints the index of the entry upon failure to make the entry easier to find.

can you please add a comment about this, it is not obvious

We usually use ImmutableMap.Builder also when we want to preserve insertion order.
ImmutableMap guarantees order.

BTW here List<Entry<..> would do. Or restructuring the test so that data is not needed at all.

presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java

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

presto-main/src/main/java/io/prestosql/sql/planner/planprinter/IoPlanPrinter.java

presto-product-tests/src/main/java/io/prestosql/tests/hive/TestHiveTableStatistics.java

presto-main/src/main/java/io/prestosql/sql/planner/planprinter/IoPlanPrinter.java

JamesRTaylor · 2019-08-16T17:49:33Z

@kokosing - thanks for the prior review. I've updated the PR based on your feedback:

removed estimatedBytesScanned as it wouldn't be accurate anyway.
squashed the commits and shortened the commit message

kokosing

Can you also post how now EXPLAIN (TYPE IO) looks now, and how it was before? So it is easy to compare how it changed.

presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java

kokosing · 2019-08-19T05:42:30Z

presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java

+        EstimatedStatsAndCost estimate16 = new EstimatedStatsAndCost(1.0, 16.0, 16.0, 0.0, 0.0, 6.0);
+        EstimatedStatsAndCost estimate17 = new EstimatedStatsAndCost(1.0, 17.0, 17.0, 0.0, 0.0, 3.0);
+        EstimatedStatsAndCost estimate25 = new EstimatedStatsAndCost(1.0, 25.0, 25.0, 0.0, 0.0, 3.0);
+        Map<Object, TypeAndEstimate> data = new LinkedHashMap<>();


can you please add a comment about this, it is not obvious

presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java

presto-product-tests/src/main/java/io/prestosql/tests/hive/TestHiveTableStatistics.java

JamesRTaylor · 2019-08-22T22:09:33Z

@kokosing - thanks for your previous reviews. I've pushed another commit based on your feedback.

presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java

presto-main/src/main/java/io/prestosql/sql/planner/planprinter/IoPlanPrinter.java

JamesRTaylor · 2019-08-26T23:22:09Z

Thanks for the feedback, @kokosing. I've pushed a new commit to address them.

kokosing

Please rebase and squash the commits. Then please extract formatting changes as separate commit before your actual change. Then I will merge this. Thanks!

kokosing · 2019-08-27T09:00:47Z

presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java

-                                                                        new FormattedMarker(Optional.of("false"), EXACTLY),
-                                                                        new FormattedMarker(Optional.of("false"), EXACTLY)))))))),
-                        Optional.of(new CatalogSchemaTableName(catalog, "tpch", "test_orders"))));
+                        ImmutableSet.of(


now when you change the formatting of existing code that is not strictly related to your change, please extract that as separate commit. Before your change.

sorry about such comments

JamesRTaylor · 2019-08-28T00:18:10Z

@kokosing - thanks for the reviews. I've pushed the changes.

kokosing · 2019-08-28T08:44:43Z

presto-main/src/main/java/io/prestosql/sql/planner/planprinter/IoPlanPrinter.java

+                                tableMetadata.getCatalogName().getCatalogName(),
+                                tableMetadata.getTable().getSchemaName(),
+                                tableMetadata.getTable().getTableName()),
+                        parseConstraints(node.getTable(), predicate)));


❤️ Thank you!

kokosing · 2019-08-28T08:48:44Z

It looks there some issue with travis, some external service was unavailable. Restarted.

JamesRTaylor · 2019-08-28T15:14:42Z

Looks like an OOM error:

[INFO] Compiling 807 source files to /home/travis/build/prestosql/presto/presto-main/target/test-classes
Terminating due to java.lang.OutOfMemoryError: GC overhead limit exceeded

Please let me know if any action is required on my side.

kokosing · 2019-08-29T07:59:55Z

Can you please raise Xmx in MAVEN_OPTS in .travis.yml to 1G.

FYI: @electrum @findepi

findepi · 2019-08-29T09:40:06Z

presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java

+        EstimatedStatsAndCost estimate16 = new EstimatedStatsAndCost(1.0, 16.0, 16.0, 0.0, 0.0, 6.0);
+        EstimatedStatsAndCost estimate17 = new EstimatedStatsAndCost(1.0, 17.0, 17.0, 0.0, 0.0, 3.0);
+        EstimatedStatsAndCost estimate25 = new EstimatedStatsAndCost(1.0, 25.0, 25.0, 0.0, 0.0, 3.0);
+        Map<Object, TypeAndEstimate> data = new LinkedHashMap<>();


We usually use ImmutableMap.Builder also when we want to preserve insertion order.
ImmutableMap guarantees order.

BTW here List<Entry<..> would do. Or restructuring the test so that data is not needed at all.

findepi · 2019-08-29T09:40:25Z

presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java

+                                    estimate)),
+                            Optional.empty(),
+                            estimate),
+                    format("%d) Type %s ", index, type));


missing ( ?

findepi · 2019-08-29T09:48:40Z

Can you please raise Xmx in MAVEN_OPTS in .travis.yml to 1G.

So far we've been seeing OOM on full maven build (with verification), and rather rarely.
This PR OOMs during compilation several times (~~https://travis-ci.com/prestosql/presto/builds/124792147~~ (re-run)). I don't see any reason why it would be so, but that's strange.

@JamesRTaylor could you please trigger a new build (rather than retriggering the failed one) so that we can see how reproducible it is. You can also trigger a build with changed MAVEN_OPTS.

eg

push an empty commit
push a commit changing MAVEN_OPTS
go to Travis and make sure the empty commit's build is run (Travis auto-cancels builds but you can still manually force a build to run)

@kokosing We can probably increase -Xmx for Maven, but need to think whether we don't go over limit somewhere else, when maven is running concurrently.
Like tests execution in a forked JVM (but tests are limited at 2g). Anything else?

kokosing · 2019-08-29T09:58:50Z

I don't see any reason why it would be so, but that's strange.

It is strange to me as well. Let me retrigger the build with flushed cache.

findepi · 2019-08-29T12:59:49Z

It is strange to me as well. Let me retrigger the build with flushed cache.

i see it green, so ... @JamesRTaylor @kokosing let's do nothing and wait until problem resurfaces.

kokosing · 2019-08-29T13:02:47Z

Merged, thanks!

cla-bot bot added the cla-signed label May 22, 2019

kokosing reviewed May 23, 2019

View reviewed changes

JamesRTaylor force-pushed the est-scanned-bytes-in-explain-io branch from 516394d to ea8a8aa Compare July 9, 2019 17:21

findepi assigned kokosing Jul 10, 2019

kokosing reviewed Jul 11, 2019

View reviewed changes

dain assigned JamesRTaylor and unassigned kokosing Aug 1, 2019

JamesRTaylor force-pushed the est-scanned-bytes-in-explain-io branch from d73e657 to ec78d26 Compare August 15, 2019 23:53

JamesRTaylor changed the title ~~Add estBytesScanned to output of EXPLAIN (TYPE IO) based on column statistics~~ Include cost estimates in IO EXPLAIN plan Aug 15, 2019

kokosing reviewed Aug 19, 2019

View reviewed changes

JamesRTaylor force-pushed the est-scanned-bytes-in-explain-io branch 3 times, most recently from 05d911c to ca9e68c Compare August 22, 2019 16:55

JamesRTaylor force-pushed the est-scanned-bytes-in-explain-io branch from 2ed895f to 26c773d Compare August 23, 2019 18:22

kokosing reviewed Aug 26, 2019

View reviewed changes

kokosing approved these changes Aug 27, 2019

View reviewed changes

Fix indentation

8b4f4b1

Include cost estimates in IO EXPLAIN plan

7b45f97

JamesRTaylor force-pushed the est-scanned-bytes-in-explain-io branch from 7aff6a5 to 7b45f97 Compare August 27, 2019 18:05

kokosing approved these changes Aug 28, 2019

View reviewed changes

findepi reviewed Aug 29, 2019

View reviewed changes

kokosing merged commit 067e865 into trinodb:master Aug 29, 2019

kokosing added this to the 319 milestone Aug 29, 2019

kokosing mentioned this pull request Aug 29, 2019

Release notes for 319 #1378

Closed

6 tasks

JamesRTaylor deleted the est-scanned-bytes-in-explain-io branch August 29, 2019 18:10

betodealmeida mentioned this pull request Sep 4, 2019

Allow users to estimate query cost before executing it apache/superset#8172

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include cost estimates in IO EXPLAIN plan #806

Include cost estimates in IO EXPLAIN plan #806

JamesRTaylor commented May 22, 2019

ebyhr commented May 23, 2019 •

edited

Loading

findepi commented May 24, 2019

ebyhr commented May 24, 2019 •

edited

Loading

JamesRTaylor commented Jun 19, 2019

JamesRTaylor commented Jul 8, 2019

kokosing commented Jul 9, 2019

JamesRTaylor commented Jul 9, 2019

findepi commented Jul 9, 2019 •

edited

Loading

JamesRTaylor commented Jul 9, 2019

JamesRTaylor commented Jul 10, 2019

kokosing Jul 11, 2019

JamesRTaylor Aug 16, 2019

kokosing Aug 19, 2019

findepi Aug 29, 2019

JamesRTaylor commented Aug 16, 2019 •

edited

Loading

kokosing left a comment

kokosing Aug 19, 2019

JamesRTaylor commented Aug 22, 2019

JamesRTaylor commented Aug 26, 2019

kokosing left a comment

kokosing Aug 27, 2019

JamesRTaylor commented Aug 28, 2019

kokosing Aug 28, 2019

kokosing commented Aug 28, 2019

JamesRTaylor commented Aug 28, 2019 •

edited

Loading

kokosing commented Aug 29, 2019

findepi Aug 29, 2019

findepi Aug 29, 2019

findepi commented Aug 29, 2019 •

edited

Loading

kokosing commented Aug 29, 2019

findepi commented Aug 29, 2019

kokosing commented Aug 29, 2019

Include cost estimates in IO EXPLAIN plan #806

Include cost estimates in IO EXPLAIN plan #806

Conversation

JamesRTaylor commented May 22, 2019

ebyhr commented May 23, 2019 • edited Loading

findepi commented May 24, 2019

ebyhr commented May 24, 2019 • edited Loading

JamesRTaylor commented Jun 19, 2019

JamesRTaylor commented Jul 8, 2019

kokosing commented Jul 9, 2019

JamesRTaylor commented Jul 9, 2019

findepi commented Jul 9, 2019 • edited Loading

JamesRTaylor commented Jul 9, 2019

JamesRTaylor commented Jul 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesRTaylor commented Aug 16, 2019 • edited Loading

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesRTaylor commented Aug 22, 2019

JamesRTaylor commented Aug 26, 2019

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesRTaylor commented Aug 28, 2019

Choose a reason for hiding this comment

kokosing commented Aug 28, 2019

JamesRTaylor commented Aug 28, 2019 • edited Loading

kokosing commented Aug 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi commented Aug 29, 2019 • edited Loading

kokosing commented Aug 29, 2019

findepi commented Aug 29, 2019

kokosing commented Aug 29, 2019

ebyhr commented May 23, 2019 •

edited

Loading

ebyhr commented May 24, 2019 •

edited

Loading

findepi commented Jul 9, 2019 •

edited

Loading

JamesRTaylor commented Aug 16, 2019 •

edited

Loading

JamesRTaylor commented Aug 28, 2019 •

edited

Loading

findepi commented Aug 29, 2019 •

edited

Loading