jayzhan211 commented on PR #24501:
URL: https://github.com/apache/datafusion/pull/24501#issuecomment-5462528890
**Satisfaction is granted in the transformed key domain, but the advertised
split points stay in the source domain**
Not a live bug as far as I can tell, but it is an invariant that other code
already depends on, so it is worth pinning down before this lands.
### The mismatch
`project` and `range_monotonic_fn_satisfaction` treat the same situation
differently:
```text
project: Range([ts], [S]) -> Range([f(ts)], [f(S)]) split
translated
satisfaction: Range([ts], [S]) vs KeyPartitioned([f(ts)]) -> Exact
... and the partitioning still advertises [S]
```
After `Exact`, the plan says "this input is partitioned by `f(ts)`", while
`Partitioning::Range` still reports ordering `ts` and split point `S`. The
effective boundary in required-key space is `f(S)`, not `S`.
For the disjointness question this does not matter, and the aggregation case
in this PR is sound. It matters for consumers that read split-point *values*
and compare them across two partitionings:
```rust
// datafusion/physical-plan/src/distribution_requirements.rs:348
(Partitioning::Range(left), Partitioning::Range(right)) => {
left.split_points() == right.split_points() && ...
```
`co_partitioning_satisfied` calls `satisfaction(..., allow_subset = false)`,
which is exactly the path that returns `Exact` here. So a join can conclude two
inputs are co-partitioned by comparing boundaries that were never in the same
domain as the keys being joined. `union.rs:1726` builds a `Range` from
children's partitionings on the same kind of equality.
### Why it does not bite today
I probed the transforms that actually clear `check_monotonic_transform` with
an unbounded source range:
| transform | monotonic? | satisfaction | `f(split) == split`? |
|---|---|---|---|
| `date_bin` / `date_trunc`, aligned split | yes | `Exact` | yes |
| `CAST(ts AS Timestamp(Second))` | yes | `Exact` | no, different
`ScalarValue` variant |
| `CAST(ts AS Date32)` | yes | `NotSatisfied` | n/a |
| `ts + INTERVAL '1 hour'`, `x + 1` | no | `NotSatisfied` | n/a |
Two things save it:
1. For `date_bin` and `date_trunc`, `f(split) != f(split-1)` means the split
*starts* its bin, so `f(split) == split`. The translated and untranslated
boundaries coincide, and the layout comparison is accidentally correct.
2. Shift-style transforms never get here. `BinaryExpr::get_properties` sends
`Plus` through `arithmetic_sort_properties`, which returns `Unordered` for
unbounded operand ranges, and `get_expr_properties` always seeds the dependency
unbounded.
The cast row is the one that already breaks the invariant: `CAST(ts AS
Timestamp(Second))` yields `Exact` with `f(split)` a `TimestampSecond` while
the advertised split stays `TimestampNanosecond`. It happens to be harmless
because those variants never compare equal, so the layout check just returns
false. That is luck, not design.
### What I would do
Cheapest option, document the assumption where it is relied upon:
```rust
// Callers may compare `RangePartitioning::split_points()` across two
// partitionings (see `compatible_co_partitioning_layout`). Those values
// stay in the range-key domain, while satisfaction here is granted in the
// transformed domain. That is only consistent because every transform
// reaching this point is a flooring function, i.e. disjointness at `split`
// implies `f(split) == split`. A monotone transform without that fixed
// point would make the advertised boundary wrong for those callers.
```
Conservative option, enforce it, at the cost of dropping the narrowing-cast
case:
```diff
let Some(at_split) = evaluate_expr_on_key(fn_expr, range_key,
split_value) else {
return false;
};
+ // The split must be a fixed point of `f`. Otherwise the boundary in
+ // the transformed domain is `f(split)`, not `split`, and callers
that
+ // compare raw split points are reasoning about the wrong value.
+ if at_split != *split_value {
+ return false;
+ }
let Some(below_split) = evaluate_expr_on_key(fn_expr, range_key,
&predecessor)
else {
return false;
};
at_split != below_split
```
Every test in the PR passes with that guard, since the 60s `date_bin` and
`date_trunc('hour')` splits are both fixed points.
Either way, a test asserting the current `CAST(ts AS Timestamp(Second))`
behaviour would make a future regression visible. Happy to see this as a
follow-up issue rather than in this PR.
--
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]
