Improve planning time for Iceberg simple SELECT queries with optional LIMIT #17347

findepi · 2023-05-04T11:12:36Z

No description provided.

...n/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMetadataFileOperations.java

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableHandle.java

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java

alexjo2144

Couple nit-picks, and test failures look relevant. Neat improvement though

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSplitSource.java

alexjo2144 · 2023-05-04T18:13:35Z

I noticed you had to override the default executor for the table scan in the tests. Is the change still useful with the default behavior?

findepi · 2023-05-05T06:29:27Z

I noticed you had to override the default executor for the table scan in the tests. Is the change still useful with the default behavior?

yes, i am using direct executor for determinism

Without that there still is improvement, but because ParallelIterable is used, the number of opened files was not deterministic.

I am still concerned about the fact we're opening 20 manifests, while 3 should suffice.
I didn't track yet where this number comes from, but it seems constant.

findepi · 2023-05-05T06:52:43Z

and test failures look relevant

I will drop Abstain from loading stats if they were not needed commit (move to #17347)
it is not needed for SELECT flow (unless someone enables collect_plan_statistics_for_all_queries, but then it's wrong).

I will add EXPLAIN ANALYZE test case too because of

trino/core/trino-main/src/main/java/io/trino/sql/planner/LogicalPlanner.java

Line 239 in e03d410

    
           return plan(analysis, stage, analysis.getStatement() instanceof ExplainAnalyze || isCollectPlanStatisticsForAllQueries(session));

line

findepi · 2023-05-05T08:11:01Z

AC

findepi · 2023-05-05T08:32:10Z

I am still concerned about the fact we're opening 20 manifests, while 3 should suffice.
I didn't track yet where this number comes from, but it seems constant.

Found it. See code comment just added (https://github.com/trinodb/trino/compare/bdd89a6efc32f7fef1ba29eeb7e60cbab8b52761..50409fc8c53c99bc4f7d5810e7a346730bd84f4c)

findepi · 2023-05-05T12:50:55Z

rebased to resolve conflict

findepi · 2023-05-05T18:02:58Z

check-commit (54302eb0c86a8130968406c38c4c0352e1610f10)

Fetching the repository
  /usr/bin/git -c protocol.version=2 fetch --prune --progress --no-recurse-submodules origin +refs/heads/*:refs/remotes/origin/* +refs/tags/*:refs/tags/*
  Error: fatal: unable to access 'https://github.com/trinodb/trino/': The requested URL returned error: 429
  The process '/usr/bin/git' failed with exit code 128
  Waiting 10 seconds before trying again
  /usr/bin/git -c protocol.version=2 fetch --prune --progress --no-recurse-submodules origin +refs/heads/*:refs/remotes/origin/* +refs/tags/*:refs/tags/*
  Error: fatal: unable to access 'https://github.com/trinodb/trino/': The requested URL returned error: 429
  The process '/usr/bin/git' failed with exit code 128
  Waiting 16 seconds before trying again
  /usr/bin/git -c protocol.version=2 fetch --prune --progress --no-recurse-submodules origin +refs/heads/*:refs/remotes/origin/* +refs/tags/*:refs/tags/*
  Error: fatal: unable to access 'https://github.com/trinodb/trino/': The requested URL returned error: 429
  Error: The process '/usr/bin/git' failed with exit code 128

ignoring

findepi · 2023-05-05T18:05:03Z

We probably would get advantages similar to achieved here without modifying the connector by tuning query.min-schedule-split-batch-size down. I think it's still good change to have in, without any downsides i can see

When `assertThat(...).containsExactlyInAnyOrderElementsOf` fails, it prints diff, requiring to count elements in the diff. Reuse more friendly assertions previously used for metastore access tests only.

There were two defaults -- one in the query runner (used by some tests) and one TEST_SESSION (used by most tests).

Previously we called `Scan.planFiles` and transformed it with `TableScanUtil.splitFiles`. The new code iterates over output from `Scan.planFiles` and splits each individually. As the advantage, this introduces a scope within which we know a file has been processed.

findepi · 2023-05-10T15:23:21Z

(rebased to resolve conflicts)

ebyhr · 2023-05-11T01:32:43Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSplitSource.java

+            if (!fileTasksIterator.hasNext()) {
+                // This is the last task for this file
+                if (!fileHasAnyDeletions) {
+                    // There were no deletions, so we produces splits covering the whole file


Suggested change

// There were no deletions, so we produces splits covering the whole file

// There were no deletions, so we produce splits covering the whole file

thanks! changing to "produced"

findepi requested review from pettyjamesm, ebyhr, alexjo2144 and findinpath May 4, 2023 11:12

cla-bot bot added the cla-signed label May 4, 2023

findepi force-pushed the findepi/select-limit branch 2 times, most recently from f27f169 to 1c646c2 Compare May 4, 2023 11:20

github-actions bot added delta-lake Delta Lake connector hive Hive connector iceberg Iceberg connector tests:hive labels May 4, 2023

findepi force-pushed the findepi/select-limit branch 3 times, most recently from bb48796 to 5e8ead4 Compare May 4, 2023 12:39

findinpath reviewed May 4, 2023

View reviewed changes

...n/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMetadataFileOperations.java Show resolved Hide resolved

findinpath reviewed May 4, 2023

View reviewed changes

...n/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMetadataFileOperations.java Show resolved Hide resolved

pettyjamesm reviewed May 4, 2023

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableHandle.java Show resolved Hide resolved

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java Outdated Show resolved Hide resolved

alexjo2144 reviewed May 4, 2023

View reviewed changes

findepi mentioned this pull request May 5, 2023

Do not force-load stats for EXPLAIN & EXPLAIN ANALYZE #17355

Closed

findepi force-pushed the findepi/select-limit branch from 5e8ead4 to bdd89a6 Compare May 5, 2023 07:46

findepi force-pushed the findepi/select-limit branch from bdd89a6 to 50409fc Compare May 5, 2023 08:31

findepi requested review from findinpath, alexjo2144 and pettyjamesm May 5, 2023 10:20

findepi force-pushed the findepi/select-limit branch from 50409fc to 987c8d8 Compare May 5, 2023 12:50

findepi added 7 commits May 10, 2023 17:21

Improve assertion message in file access tests

e36a9f5

When `assertThat(...).containsExactlyInAnyOrderElementsOf` fails, it prints diff, requiring to count elements in the diff. Reuse more friendly assertions previously used for metastore access tests only.

Reorder file types according to Iceberg table structure

3e98906

Do not load unnecessary test data in TestIcebergMetadataFileOperations

847e05d

Deduplicate test sessions in TestIcebergMetadataFileOperations

9ad8de7

There were two defaults -- one in the query runner (used by some tests) and one TEST_SESSION (used by most tests).

Test Iceberg planning file accesses with LIMIT

4d12ac4

Reorder fields in IcebergSplitSource in use order

a7089e4

findepi force-pushed the findepi/select-limit branch from 987c8d8 to 0f82b0f Compare May 10, 2023 15:23

ebyhr approved these changes May 11, 2023

View reviewed changes

Push LIMIT into Iceberg connector

6dc58d1

findepi force-pushed the findepi/select-limit branch from 0f82b0f to 6dc58d1 Compare May 11, 2023 20:00

findepi merged commit a30ee90 into trinodb:master May 11, 2023

findepi deleted the findepi/select-limit branch May 11, 2023 20:06

github-actions bot added this to the 418 milestone May 11, 2023

colebow mentioned this pull request May 17, 2023

Add Trino 418 release notes #17540

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve planning time for Iceberg simple SELECT queries with optional LIMIT #17347

Improve planning time for Iceberg simple SELECT queries with optional LIMIT #17347

findepi commented May 4, 2023

alexjo2144 left a comment

alexjo2144 commented May 4, 2023

findepi commented May 5, 2023

findepi commented May 5, 2023 •

edited

Loading

findepi commented May 5, 2023

findepi commented May 5, 2023

findepi commented May 5, 2023

findepi commented May 5, 2023

findepi commented May 5, 2023

findepi commented May 10, 2023

ebyhr May 11, 2023

findepi May 11, 2023

	// There were no deletions, so we produces splits covering the whole file
	// There were no deletions, so we produce splits covering the whole file

Improve planning time for Iceberg simple SELECT queries with optional LIMIT #17347

Improve planning time for Iceberg simple SELECT queries with optional LIMIT #17347

Conversation

findepi commented May 4, 2023

alexjo2144 left a comment

Choose a reason for hiding this comment

alexjo2144 commented May 4, 2023

findepi commented May 5, 2023

findepi commented May 5, 2023 • edited Loading

findepi commented May 5, 2023

findepi commented May 5, 2023

findepi commented May 5, 2023

findepi commented May 5, 2023

findepi commented May 5, 2023

findepi commented May 10, 2023

ebyhr May 11, 2023

Choose a reason for hiding this comment

findepi May 11, 2023

Choose a reason for hiding this comment

findepi commented May 5, 2023 •

edited

Loading