gene-bordegaray commented on PR #23184:
URL: https://github.com/apache/datafusion/pull/23184#issuecomment-4935078164
> LGTM, Thank you!
>
> ### Alternative Design
> Iād like to share a design idea I have in mind. If we were implementing
this from scratch, I suspect this alternative might be simpler. I understand
this is not feasible in a single PR, and the current approach is practical and
quite good already. So this idea is only to provide additional perspectives.
>
> The current design seems to treat per-child distribution requirements and
cross-child requirements as separate mechanisms, then handles them individually.
>
> An alternative approach:
>
> * Treat co-partitioning as the top-level requirement to satisfy.
> * Make `enforce_distribution` do one thing: given the current source
physical layout, config values, and statistics, decide the cheapest operator(s)
to insert.
>
> I think this maps more directly to the essence of the problem, so the
implementation might become simpler. Separating these concepts may require
keeping implicit consistency across different places, and the mechanisms can
sometimes cancel each other out.
>
> ```
> enum DistributionRequirement {
> // Single-child case
> Distribution(Distribution),
>
> // Multi-child co-partitioning case
> CoPartition(CoPartition),
> }
> ```
>
> For example:
>
> ```
> -- t1 and t2 are both source tables backed by Parquet
> SELECT *
> FROM t1
> INNER JOIN t2
> ON t1.v1 = t2.v1;
> ```
>
> Suppose the physical layout is:
>
> ```
> # t1: 4 similarly sized range partitions
> t1:
> p1.parquet # v1 range [0, 100)
> p2.parquet # v1 range [100, 200)
> p3.parquet # v1 range [200, 300)
> p4.parquet # v1 range [300, 400)
>
> # t2: one partition, uniform distribution over [0, 400)
> t2:
> p1.parquet # v1 range [0, 400)
> ```
>
> And the configuration is:
>
> ```
> target_partitions = 2
> ```
>
> In this model, `enforce_distribution` would look at the physical layout,
configuration, and statistics, and could decide to:
>
> * Optimize `t1`ās scan operator into this shape:
>
> * partition 1: `[p1.parquet, p2.parquet]`
> * partition 2: `[p3.parquet, p4.parquet]`
> * add `RepartitionExec(range, split_points = [200])` above the `t2` scan
operator
>
> If we first satisfy the per-child requirements independently, it seems
harder to reach this optimal plan without adding many special cases, and we may
end up with an extra hash repartition on `v1`.
>
> ### Related Issue
> One related issue is that we currently enforce sorting and distribution
through separate mechanisms. It looks like they can cancel each other out,
which makes the implementation harder to maintain.
>
> The essence of the issue is the same: given the input plan and required
plan properties, find the cheapest plan change that satisfies the requirement.
Unifying these mechanisms would likely simplify the overall implementation too,
but that looks like an even larger refactor.
>
> * #21973
@2010YOUY01 I see what you are saying here, and I think this is a great
long-term model to have in mind. It seems like what you are saying is the
current contract is split into two:
1. Each child satisfies their individual distributions
2. Then they are aligned to be co-partitioned
This is a good design with what we are given, it would be better to just
explicitly say upfront that these children need to be co-partitioned and derive
that immediately.
With the current design I could see us moving toward this shape cleanly. We
could introduce something like you suggested
```rust
enum InputDistributionRequirement {
PerChild {...},
CoPartitioned {...},
}
```
and then rather than operators asking for a Distribution they can directly
ask for per child or co-partitioned requirements. I think this would, again
like you suggested, be pretty big PR (or PRs) but I am up to keep looking into
this š
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
