jayzhan211 commented on issue #27429:
URL: https://github.com/apache/datafusion/issues/27429#issuecomment-6894773657
@marvelefe following up on my note above about the tests: one change to the
plan. My prototype formats the facts in a new function, `equivalence_facts`, in
`physical-plan/src/display.rs`. Could you put that formatting in
`datafusion-physical-expr` instead, by rewriting the existing `impl Display for
EquivalenceProperties`, so that `EXPLAIN` only needs roughly:
```rust
write!(f, ", facts=[{}]", plan.equivalence_properties())?;
```
The `EXPLAIN` output stays the same; only where the formatting code lives
changes. The rest of your plan looks good.
The reason is the transcript-style tests. The equivalence unit tests are in
`physical-expr`, which can't call into `physical-plan`, so with the formatting
in `physical-plan` they would need a second copy, and the two would drift
apart. (If the tree format needs separate keys, a method in `physical-expr`
that returns key/value pairs works too.) For your PR, the `EXPLAIN`
sqllogictests with the option on are already transcripts, and the formatting's
own unit tests can be `insta` snapshots. Rewriting the existing equivalence
tests in this style can be a separate PR.
The existing
[`Display`](https://github.com/apache/datafusion/blob/73e0b0339ea0b67b1389ad5e6c79cf6103b47a5e/datafusion/physical-expr/src/equivalence/properties/mod.rs#L1904)
is meant to be the readable form but has the NULL problem from the issue:
`order: [[a@0 ASC]]` there means NULLS FIRST. Nothing outside tests prints it
(test failure messages, one assert in `dependency.rs`, one doc example), so
changing its format updates two expected strings.
<details><summary>What a unit test looks like with the shared formatting
(local prototype)</summary>
The sort-merge join case from #25052, on `main`:
```
-- left key a may repeat
left: order: [a@0 ASC NULLS FIRST]
right: order: [x@0 ASC NULLS FIRST, y@1 ASC NULLS FIRST]
join: Inner, on a@0 = x@0, maintains_input_order=[true, false], probe side
left
output: order: [a@0 ASC NULLS FIRST, x@2 ASC NULLS FIRST, y@3 ASC NULLS
FIRST]; equal: {a@0, x@2}
```
With the fix from #25053 applied, the snapshot diff is one line, and the
unique-key case next to it does not change:
```diff
-output: order: [a@0 ASC NULLS FIRST, x@2 ASC NULLS FIRST, y@3 ASC NULLS
FIRST]; equal: {a@0, x@2}
+output: order: [a@0 ASC NULLS FIRST]; equal: {a@0, x@2}
```
</details>
--
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]
