Preferred write partitioning threshold

[skrzypo987]

No description provided.

[skrzypo987]

To be honest I have no idea where to put it here, so it landed at the beginning.

[findepi]

@martint suggestions?

[sopel39]

Move it before or after reorder joins is applied. The assumption is that for reorder joins we should have the most accurate stats:

• PredicatePushDown has been run with useTableProperties
• later in the planning exchange nodes are added which might confuse stats.

Please add a comment why there.

[skrzypo987]

Docs still missing

[tooptoop4]

@skrzypo987 does this fix #6570 ?

[skrzypo987]

@skrzypo987 does this fix #6570 ?

I am afraid if enabling use_preferred_write_partitioning does not work then this will not work either

[skrzypo987]

I noticed that use-preferred-write-partitioning is not mentioned in the docs, just in release notes. Should this follow the same pattern?

[findepi]

Would it be possible to make the configuration toggle a little bit automatic?
I.e. how is the user going to determine whether they want to set this?
If this depends e.g. on number of partitions vs number of nodes, we could encode this reasoning within server's logic

[skrzypo987]

The idea was to leave it backward-compatible (0 as the default value) and let the user decide the threshold based on his/her workload if necessary.
However making this more "intelligent" is not a bad idea. The question is whether we want to do it here or as a follow-up.

[sopel39]

when number of written or when the number of written? I'm not native, but I think the latter is correct

[sopel39]

If this depends e.g. on number of partitions vs number of nodes, we could encode this reasoning within server's logic

This doesn't really depend on number of workers. The hard limit is hive.max-partitions-per-writers which is independent of workers.

[sopel39]

when number of written or when the number of written? I'm not native, but I think the latter is correct

[sopel39]

If this depends e.g. on number of partitions vs number of nodes, we could encode this reasoning within server's logic

This doesn't really depend on number of workers. The hard limit is hive.max-partitions-per-writers which is independent of workers.

[sopel39]

I suggest to set it to some default value, e.g: 50. This should:

• make preferred partitioning to be used when there is full table CTAS with more than 50 partitions
• provide good concurrency (>50) for preferred partitioning
• mitigate hive writer partition limit (100)
• provide good concurrency for inserts into single partition

WDYT: @findepi @willmostly ?

[sopel39]

I noticed that use-preferred-write-partitioning is not mentioned in the docs, just in release notes. Should this follow the same pattern?

Nope. Both should be documented.

[skrzypo987]

Added docs and updated the code.
Things still pending:

• Default threshold value
• Where to put the PreferWritePartitioning class in the optimizer (or leave it as it is)

[sopel39]

mechansm -> optimization

Elaborate more what partitions it refers to.

[sopel39]

Make sure it's possible to force preferred partitioning even when stats are missing (e.g when threshold is set to <=1).
When stats are missing and threshold is >1, we should fallback to not use preferred partitioning (previous default).

[sopel39]

we should also enable use-preferred-write-partitioning by default (separate commit).
Maybe we should also rename that property to use-automatic-preferred-write-partitioning and mark the old one as legacy since the previous behavior is changing. wdyt @findepi ?

[skrzypo987]

Another version.
Handles no stats/nan case gracefully. See TestPreferWritePartitioning for a list of cases.
Docs updated, however waiting for #7043 to be finally polished.
There is, however, a problem with naming - min number of partitions implies < logic, but it actually is <=. At this point we can use < since 0 is explicitly handled and keep the min in the name.
@sopel39

[sopel39]

follow up PR. I would like @mosabua to review it.

[skrzypo987]

Maybe a follow-up commit? Features and docs should IMO be added atomically.

[sopel39]

if the number of partitions cannot be estimated or preferred-write-partitioning-min-number-of-partitions > 1 then preferred write partitioning won't be used. Setting preferred-write-partitioning-min-number-of-partitions to value less or equal to 1 will force Trino to use preferrered write partitioning

[skrzypo987]

or preferred-write-partitioning-min-number-of-partitions > 1 then preferred write partitioning won't be used means that setting this value > 1 will always disable the feature. And this is not the case.
Let's wait for @mosabua to comment on that.

[sopel39]

There is, however, a problem with naming - min number of partitions implies < logic, but it actually is <=. At this point we can use < since 0 is explicitly handled and keep the min in the name.

I understand it that if estimated number of partitions >= min threshold, we use preferred write partitioning.

[sopel39]

lgtm % comments

[sopel39]

testCreateTableAsSelectUnpartitioned should use withPreferredPartitioning. Please extract as a separate commit

[sopel39]

lgtm % comment about rule placing, let's enable preferred partitioning by default

[sopel39]

Move it before or after reorder joins is applied. The assumption is that for reorder joins we should have the most accurate stats:

• PredicatePushDown has been run with useTableProperties
• later in the planning exchange nodes are added which might confuse stats.

Please add a comment why there.

[sopel39]

small comment

[sopel39]

nit: we could remove RemoveRedundantIdentityProjections from both comments (here and below) as it should not affect stats. This could be separate PR

[skrzypo987]

I'll handle it when I get some free cycles