Implement partitioned tpch/tpcds cbo plan test by gaurav8297 · Pull Request #11738 · trinodb/trino

gaurav8297 · 2022-03-31T16:50:07Z

Description

Issue: #11466

Is this change a fix, improvement, new feature, refactoring, or other?

Improvement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

CBO and benchto

How would you describe this change to a non-technical end user or system administrator?

This includes two major changes

Instead of using tpch/tpcds connector, we are now using a hive connector with in-memory metastore to run CBO plan validation tests. This essentially helps to depict the reality that is actual plans generated on the benchmark cluster. For instance, the algorithm used in hive metastore to calculate partition statistics is different from tpch/tpcds connectors.
Implemented CBO plan test for partitioned tpch/tpcds tables.

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

.../trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/InMemoryThriftMetastore.java

testing/trino-benchto-benchmarks/src/test/resources/statistics/tpcds/sf1000/README.txt

...g/trino-benchto-benchmarks/src/test/java/io/trino/sql/planner/AbstractCostBasedPlanTest.java

...ng/trino-benchto-benchmarks/src/test/java/io/trino/sql/planner/InMemoryMetastoreFactory.java

...enchto-benchmarks/src/test/java/io/trino/sql/planner/TestTpchUnPartitionedCostBasedPlan.java

...-benchto-benchmarks/src/test/java/io/trino/sql/planner/TestTpchPartitionedCostBasedPlan.java

testing/trino-benchto-benchmarks/src/test/resources/statistics/tpcds/sf1000/README.md

...no-benchto-benchmarks/src/test/java/io/trino/sql/planner/AbstractTpcdsCostBasedPlanTest.java

gaurav8297 · 2022-04-06T17:29:39Z

@raunaqmorarka PTAL

martint · 2022-04-06T17:49:42Z

Have we looked at making the TPC-H and TPC-DS connectors produce partitioned tables instead? I'm concerned about the amount of coupling between the benchmarks module and the Hive connector that this change is introducing. It will make it much harder to evolve the Hive connector in the future.

gaurav8297 · 2022-04-06T18:20:58Z

Have we looked at making the TPC-H and TPC-DS connectors produce partitioned tables instead?

We started with this but then the way hive connector fetches statistics for partitioned tables is different. It uses partition sampling and then estimates statistics across different samples. It also stores statistics in a different format PartitionStatistics and converts it back to TableStatistics. So, we thought it's better to use the hive connector itself to depict the reality which is how we are running benchmarks on the benchmark cluster.

I'm concerned about the amount of coupling between the benchmarks module and the Hive connector that this change is introducing. It will make it much harder to evolve the Hive connector in the future.

IIUIC, the majority of coupling is introduced to generate the gzip statistics files in GlueStatisticsGenerator, not for the actual tests. So, one option is we could somehow remove the GlueStatisticsGenerator, or we could keep it as it is and change the GlueStatisticsGenerator whenever there are changes in the hive connector. Do you think it'll be a big problem?

cc @raunaqmorarka @sopel39 @martint

sopel39 · 2022-04-06T19:15:16Z

@gaurav8297 Have you checked RecordingHiveMetastore? It's purpose is to dump metastore metadata. Maybe it can be used to save/load statistics for plan tests? Gzipping there (HiveMetastoreRecording#writeRecording) shouldn't be an issue.

...enchto-benchmarks/src/test/java/io/trino/sql/planner/statistics/GlueStatisticsGenerator.java

gaurav8297 · 2022-04-07T23:58:24Z

@raunaqmorarka PTAL again

...hto-benchmarks/src/test/java/io/trino/sql/planner/TestHivePartitionedTpcdsCostBasedPlan.java

...chto-benchmarks/src/test/java/io/trino/sql/planner/TestHivePartitionedTpchCostBasedPlan.java

...ino-benchto-benchmarks/src/test/java/io/trino/sql/planner/AbstractHiveCostBasedPlanTest.java

testing/trino-benchto-benchmarks/src/test/java/io/trino/sql/planner/HiveMetadataRecorder.java

testing/trino-benchto-benchmarks/src/test/resources/hive.properties

...g/trino-benchto-benchmarks/src/test/java/io/trino/sql/planner/TestHiveTpchCostBasedPlan.java

testing/trino-benchto-benchmarks/src/test/java/io/trino/sql/planner/HiveMetadataRecorder.java

...g/trino-benchto-benchmarks/src/test/java/io/trino/sql/planner/AbstractCostBasedPlanTest.java

gaurav8297 · 2022-04-12T08:38:06Z

@raunaqmorarka PTAL

raunaqmorarka

lgtm % minor comments

...g/trino-benchto-benchmarks/src/test/java/io/trino/sql/planner/AbstractCostBasedPlanTest.java

testing/trino-benchto-benchmarks/src/test/java/io/trino/sql/planner/HiveMetadataRecorder.java

...ino-benchto-benchmarks/src/test/java/io/trino/sql/planner/AbstractHiveCostBasedPlanTest.java

...-hive/src/test/java/io/trino/plugin/hive/metastore/recording/TestRecordingHiveMetastore.java

.../trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/InMemoryThriftMetastore.java

In case of huge partitioned tables, the recording file could be huge in size due to partition level statistics. So, it's better to compress the recording file which essentially makes read/write faster.

...ino-benchto-benchmarks/src/test/java/io/trino/sql/planner/AbstractHiveCostBasedPlanTest.java

sopel39

nit: we might consider renaming packages here in the future as it's more related to Hive stats now rather than generic TPCH/TPCDS connector

sopel39 · 2022-04-12T12:46:32Z

testing/trino-benchto-benchmarks/src/test/resources/hive_metadata/README.md

So, there are no min/max statistics for char-based columns.

Why?

I don't think we support min/max statistics for varchar and char columns in hive connector.

https://github.com/trinodb/trino/blob/master/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreUtil.java#L961

Q: This change was from 2018. Is this still a case that min/max for char columns are not used by the optimizer?

See StatsUtil#toStatsRepresentation, it's still the case, we rely more on NDV in that case

testing/trino-benchto-benchmarks/src/test/resources/hive_metadata/README.md

.../trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/InMemoryThriftMetastore.java

gaurav8297 · 2022-04-13T09:03:56Z

@sopel39 PTAL

sopel39

lgtm % comments

testing/trino-benchto-benchmarks/src/main/resources/sql/presto/tpcds/q75.sql

.../trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/InMemoryThriftMetastore.java

testing/trino-testing/src/main/java/io/trino/testing/AbstractTestQueryFramework.java

Currently recording metastore caches partition statistics and values for a set of partition rather than per each partition.

Instead of using connectorTableHandle, use TableMetadata to find table name in a generic way in JoinOrderPrinter.

Instead of using tpch/tpcds connector, use in-memory hive metastore with corresponding tables to depict the reality that is actual plans generated on the benchmark cluster. For instance, the algorithm used in hive metastore to calculate partition statistics is different from tpch/tpcds connectors.

sopel39 · 2022-04-14T12:29:01Z

Failed due to #11929

cla-bot bot added the cla-signed label Mar 31, 2022

gaurav8297 requested review from raunaqmorarka and sopel39 March 31, 2022 16:53

github-actions bot added the tests:hive label Mar 31, 2022

findepi reviewed Apr 1, 2022

View reviewed changes

.../trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/InMemoryThriftMetastore.java Outdated Show resolved Hide resolved

findepi reviewed Apr 1, 2022

View reviewed changes

testing/trino-benchto-benchmarks/src/test/resources/statistics/tpcds/sf1000/README.txt Outdated Show resolved Hide resolved

raunaqmorarka reviewed Apr 4, 2022

View reviewed changes

gaurav8297 marked this pull request as ready for review April 6, 2022 17:29

raunaqmorarka reviewed Apr 6, 2022

View reviewed changes

...enchto-benchmarks/src/test/java/io/trino/sql/planner/statistics/GlueStatisticsGenerator.java Outdated Show resolved Hide resolved

raunaqmorarka reviewed Apr 7, 2022

View reviewed changes

...enchto-benchmarks/src/test/java/io/trino/sql/planner/statistics/GlueStatisticsGenerator.java Outdated Show resolved Hide resolved

gaurav8297 requested a review from raunaqmorarka April 8, 2022 06:28

raunaqmorarka reviewed Apr 8, 2022

View reviewed changes

gaurav8297 requested a review from raunaqmorarka April 8, 2022 14:27

raunaqmorarka reviewed Apr 9, 2022

View reviewed changes

...g/trino-benchto-benchmarks/src/test/java/io/trino/sql/planner/AbstractCostBasedPlanTest.java Outdated Show resolved Hide resolved

raunaqmorarka approved these changes Apr 12, 2022

View reviewed changes

sopel39 reviewed Apr 12, 2022

View reviewed changes

...-hive/src/test/java/io/trino/plugin/hive/metastore/recording/TestRecordingHiveMetastore.java Outdated Show resolved Hide resolved

.../trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/InMemoryThriftMetastore.java Outdated Show resolved Hide resolved

Implement gzip hive metastore recording file

b49fbd8

In case of huge partitioned tables, the recording file could be huge in size due to partition level statistics. So, it's better to compress the recording file which essentially makes read/write faster.

sopel39 reviewed Apr 12, 2022

View reviewed changes

...ino-benchto-benchmarks/src/test/java/io/trino/sql/planner/AbstractHiveCostBasedPlanTest.java Outdated Show resolved Hide resolved

sopel39 reviewed Apr 12, 2022

View reviewed changes

sopel39 approved these changes Apr 13, 2022

View reviewed changes

gaurav8297 added 3 commits April 14, 2022 12:32

Record value per partition in Recording metastore

c7bc216

Currently recording metastore caches partition statistics and values for a set of partition rather than per each partition.

Use table metadata to get table name in CBO tests

56bc460

Instead of using connectorTableHandle, use TableMetadata to find table name in a generic way in JoinOrderPrinter.

Implement partitioned tpch/tpcds cbo plan test

4e01b1c

sopel39 merged commit ef09c41 into trinodb:master Apr 14, 2022

github-actions bot added this to the 378 milestone Apr 14, 2022

mosabua mentioned this pull request Apr 14, 2022

Add Trino 378 release notes #11962

Merged

Conversation

gaurav8297 commented Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues, pull requests, and links

Documentation

Release notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gaurav8297 commented Apr 6, 2022

Uh oh!

martint commented Apr 6, 2022

Uh oh!

gaurav8297 commented Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sopel39 commented Apr 6, 2022

Uh oh!

Uh oh!

Uh oh!

gaurav8297 commented Apr 7, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gaurav8297 commented Apr 12, 2022

Uh oh!

raunaqmorarka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sopel39 left a comment

Choose a reason for hiding this comment

Uh oh!

sopel39 Apr 12, 2022

Choose a reason for hiding this comment

Uh oh!

gaurav8297 Apr 12, 2022 • 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!

raunaqmorarka Apr 14, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gaurav8297 commented Apr 13, 2022

Uh oh!

sopel39 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sopel39 commented Apr 14, 2022

Uh oh!

gaurav8297 commented Mar 31, 2022 •

edited

Loading

gaurav8297 commented Apr 6, 2022 •

edited

Loading

gaurav8297 Apr 12, 2022 •

edited

Loading