jayzhan211 commented on code in PR #26836:
URL: https://github.com/apache/datafusion/pull/26836#discussion_r4412677763
##########
datafusion/physical-expr/src/equivalence/class.rs:
##########
@@ -792,9 +794,21 @@ impl EquivalenceGroup {
// TODO: This function should be able to return values of non-literal
// complex constants as well; e.g. it should return `8` for the
// expression `3 + 5`, not an unknown `heterogenous` value.
+ // A volatile expression can change from row to row, even when all of
+ // its inputs are constant
+ if expr.is_volatile_node() {
+ return None;
+ }
let children = expr.children();
if children.is_empty() {
- return None;
+ // A leaf that reads no column can still be constant, e.g. an
+ // uncorrelated scalar subquery, which reports itself as a
+ // singleton. It does not depend on the row or the partition, so
+ // its value is the same everywhere, even if it is not known yet.
+ let singleton = expr
+ .get_properties(&[])
+ .is_ok_and(|props| props.sort_properties ==
SortProperties::Singleton);
+ return singleton.then_some(AcrossPartitions::Uniform(None));
Review Comment:
@raghavyadav01 thanks for the fix! Treating the subquery as a constant now
reaches an existing gap in `EquivalenceGroup::join`: it keeps the left side's
constants for `Right` and `Full` joins, even though that side is padded with
NULLs (#26042 would also fix it, but it is still open). A `LEFT JOIN` gets
swapped into a physical `Right` join with the subquery on the left, the sort on
`m` is dropped, and the NULL rows come back in the middle. The same query
returns the right order on main:
```sql
SET datafusion.execution.batch_size = 1;
CREATE TABLE p (a INT, b INT) AS VALUES (1, 10), (2, 20), (3, 30), (2, 5),
(NULL, 7);
SELECT x.a, s.a, s.m
FROM p x LEFT JOIN (SELECT a, (SELECT max(a) FROM p) AS m FROM p WHERE a >
1) s ON x.a = s.a
ORDER BY s.m, x.a, s.a;
-- expected: 2 2 3 (x4), 3 3 3, 1 NULL NULL, NULL NULL NULL
-- this PR: 2 2 3, 2 2 3, 3 3 3, 1 NULL NULL, 2 2 3, 2 2 3, NULL NULL NULL
```
Clearing the left side's constants the same way as the right side's fixes
it. With this change, the query above and the full sqllogictest suite pass:
```diff
let mut result = Self::new(
- self.iter().cloned().chain(
+ self.iter()
+ .cloned()
+ .map(|mut cls| {
+ // Rows without a match on the left side are
+ // padded with NULLs, so its constants no
+ // longer hold.
+ if matches!(join_type, JoinType::Right |
JoinType::Full) {
+ cls.constant = None;
+ }
+ cls
+ })
+ .chain(
```
Could you add the query above to `subquery.slt`?
##########
datafusion/physical-expr/src/equivalence/class.rs:
##########
@@ -1364,6 +1378,117 @@ mod tests {
/// A constant on the right side of a join stays constant in the join
/// output, unless unmatched rows pad the right side with NULLs.
+ /// An uncorrelated scalar subquery is a leaf with no columns, so the only
Review Comment:
Optional cleanup: the new test is inserted between
`test_join_keeps_right_constants` and its doc comment, so that test loses its
doc and this one gets both paragraphs. Could you move the two "A constant on
the right side of a join…" lines back down, just above `///
<https://github.com/apache/datafusion/issues/26283>`?
--
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]
