gene-bordegaray commented on PR #24501:
URL: https://github.com/apache/datafusion/pull/24501#issuecomment-5589790676
Ok here is a more composable stack of work to introduce this feature in a
way that is more digesteable and allows more of the community to have eyes and
an opinion on this as there are many hidden implications in this change:
### 1. date_trunc correctness fix (pre-requisite)
There is unchecked arithmetic in
`general_date_trunc_array_fine_granularity`. This is a prerequisite because the
optimizer evaluates scalar boundary values when deciding whether `date_trunc`
preserves a range partitioning. The scalar path uses checked arithmetic, but
the array fast path can overflow or panic for `i64::MIN`.
This is an independent correctness and we need to fix this unless there is a
motivation for this in the fast-path I am unaware of.
### 2. Audit Range transformation sand projections
I believe that preservation for functions should be default-deny. An
ordinary `Ordered` property is not enough to confidently declare a function
preserves partition boundaries. There can be nasty edge cases and opting into
everything at once is scary.
I think we should:
- thoroughly audit `date_bin` and `date_trunc` timestamp units, time zones,
NULLs, and all these weird cases
- only project split points through thoroughly tested transformations,
otherwise reject
### 3. Distinguish transformed satisfaction from Exact, then add the first
aggregation consumer
`Satisfaction::Exact` means that the partition key and required key are
exact matches. This is strict enough for co-partitioned operators, like
partitioned joins, because their split points can be compared directly to see,
well if they are exactly matching.
Now, a transformed key is not as strong. Its weaker guarantee is that equal
`f(key)` values may stay local to one input partition. This makes things like
not shuffling on aggregation safe, but now the boundary is `f(split)`, not the
original split points.
For example:
```text
left: Range(x), split 10, join key x + 1
right: Range(y), split 10, join key y
```
The effective boundaries are 11 and 10. Without repartitioning, x = 9 and y
= 10 are placed in different partition indexes even though x + 1 = y.
Co-partitioning is quite tricky and we need to be careful when eliminating
shuffles on opertors that require it, it is much stricter than just satisfying
`Distribution`s
So there may be need for discussion around if this needs a new variant of
satisfaction. Single-input aggregates may accept it, while co-partitioned joins
should continue to require Exact layout compatibility.
I also think this PR should also add the first consumer, which can be the
no-repartition aggregate optimization as I have found it is usually better to
have a direct consumer of an API change rather than writing speculative code.
--
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]
