Avoid unnecessary DeterminePartitionCount work #17150
Conversation
pettyjamesm
force-pushed
the
limit-determine-partition-count
branch
from
fa552f9 to
178e1fd
Compare
pettyjamesm
force-pushed
the
limit-determine-partition-count
branch
from
178e1fd to
56bdc11
Compare
|
There are some test failures. Can you look into that? |
pettyjamesm
force-pushed
the
limit-determine-partition-count
branch
from
56bdc11 to
75fa8b4
Compare
Yep, test failures are related because some tests are asserting the number of metastore accesses which is now lower than before for some queries. I've updated the assertions that I could verify locally with the new numbers, but may still be missing some assertions so we'll have to wait for the next CI run results to see if there are more updates needed. |
github-actions
bot
added
delta-lake
pettyjamesm
force-pushed
the
limit-determine-partition-count
branch
from
75fa8b4 to
9ade84c
Compare
...trino-main/src/test/java/io/trino/sql/planner/optimizations/TestDeterminePartitionCount.java
Show resolved
Hide resolved
...trino-main/src/test/java/io/trino/sql/planner/optimizations/TestDeterminePartitionCount.java
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/sql/planner/optimizations/DeterminePartitionCount.java
Show resolved
Hide resolved
...trino-main/src/test/java/io/trino/sql/planner/optimizations/TestDeterminePartitionCount.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/sql/planner/optimizations/DeterminePartitionCount.java
Outdated
Show resolved
Hide resolved
Modifies the DeterminePartitionCount optimizer rule to only attempt stats collection and dynamic partition count selection when the plan contains an eligible remote exchange. Otherwise, plan stats collection would be triggered potentially unnecessarily.
pettyjamesm
force-pushed
the
limit-determine-partition-count
branch
from
9ade84c to
133067d
Compare
core/trino-main/src/main/java/io/trino/sql/planner/optimizations/DeterminePartitionCount.java
Show resolved
Hide resolved
| return false; | ||
| } | ||
| PartitioningHandle partitioningHandle = exchangeNode.getPartitioningScheme().getPartitioning().getHandle(); | ||
| return !partitioningHandle.isScaleWriters() |
There was a problem hiding this comment.
nit: this is probably enforced by PlanNodeSearcher.searchFrom(plan).whereIsInstanceOfAny(INSERT_NODES).matches(); check.
There was a problem hiding this comment.
It was until #17024 enabled automatic partition determination for write queries behind a session property (default: false).
| assertDistributedPlan( | ||
| query, | ||
| Session.builder(getQueryRunner().getDefaultSession()) | ||
| .setSystemProperty(MAX_HASH_PARTITION_COUNT, "20") |
There was a problem hiding this comment.
I guess you could move it as default session setup for these tests
There was a problem hiding this comment.
I think it's ok to explicitly configure the methods separately since many of the tests still use different values to assert different count values selected from the parameters used.
Description
Modifies the
DeterminePartitionCountoptimizer rule to only attempt stats collection and dynamic partition count selection when the plan contains an eligible remote exchange. Otherwise, plan stats collection would be triggered potentially unnecessarily.Release notes
(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: