refactor(optimizer): split required and provided distribution property

[st1page]

What's changed and what's your intention?

rethinking the distribution bugs(#2447 #1926 #1653) and strange code and think the current implementation of distribution property mix the required distribution and provided distribution up.
split required and provided distribution property.
see the comments in src/frontend/src/optimizer/property/distribution.rs for more information.

the planner test's diff is due to

• after changing the RequiredDist::ShardByKey with the order-irrelevance bit-set, the exchange shuffle key's order changes
• in this PR we prefer project before the exchange because it will save many IO in exchange in our current implementation

Checklist

• I have written necessary docs and comments
• I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

[fuyufjh]

The idea LGTM

[liurenjie1024]

Do we also need to add RequiredOrder and ProvidedOrder? Another design is like following:

enum DistributionSpec {
Any,
...
}

enum OrderSpec {
Any,
...
}

struct RequiredPhysicalProperty {
   dist_spec: DistributionSpec,
   order_spec: OrderSpec
}

struct DerivedPhysicalProperty {
   dist_spec: DistributionSpec,
   order_spec: OrderSpec
}

[st1page]

Do we also need to add RequiredOrder and ProvidedOrder? Another design is like following:

• I think we need to add RequiredOrder and ProvidedOrder when we actually use the order in optimizing.
• I think the "Another design" is independent of this PR, we should split the DistributionSpec to DistributionSpec DistributionRequire to get the type safety and other benefits

[st1page]

I think maybe there is a bug. if our batch execution can handle exchange with AnyShard? @liurenjie1024

[liurenjie1024]

Currently no.

[codecov]

Codecov Report

Merging #2726 (a453f14) into main (c45c2f9) will increase coverage by 0.00%.
The diff coverage is 88.88%.

@@           Coverage Diff           @@
##             main    #2726   +/-   ##
=======================================
  Coverage   72.47%   72.48%           
=======================================
  Files         697      697           
  Lines       90329    90358   +29     
=======================================
+ Hits        65468    65498   +30     
+ Misses      24861    24860    -1     

Flag	Coverage Δ
rust	72.48% <88.88%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...d/src/optimizer/plan_node/batch_generate_series.rs	0.00% <0.00%> (ø)
...ntend/src/optimizer/plan_node/stream_delta_join.rs	48.07% <ø> (-0.50%)	⬇️
...ontend/src/optimizer/plan_node/stream_hash_join.rs	97.05% <ø> (-0.12%)	⬇️
...ntend/src/optimizer/plan_node/stream_simple_agg.rs	95.74% <ø> (+1.99%)	⬆️
...rc/frontend/src/optimizer/plan_node/stream_topn.rs	53.84% <ø> (+1.01%)	⬆️
...frontend/src/optimizer/plan_node/batch_hash_agg.rs	73.52% <50.00%> (-0.39%)	⬇️
src/frontend/src/utils/column_index_mapping.rs	76.85% <66.66%> (-1.79%)	⬇️
...rontend/src/optimizer/plan_node/stream_hash_agg.rs	93.33% <75.00%> (+1.14%)	⬆️
.../frontend/src/optimizer/plan_node/batch_project.rs	81.81% <81.81%> (-0.44%)	⬇️
.../src/optimizer/plan_node/batch_nested_loop_join.rs	85.18% <83.33%> (-0.27%)	⬇️
... and 31 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[skyzh]

Rest LGTM

[skyzh]

When is it possible to have round-robin? 🤣

[liurenjie1024]

To increase parallelism of processing.

[skyzh]

Round robin will cause a lot of problems. For example, in two-phase agg, it will cause row_count to be negative. I think long time ago we've decided to use hash distribution of the values for the local phase.

[skyzh]

But it's okay to have it in batch, IMHO.

[liurenjie1024]

Yes, it should be used carefully.