Skip to content

Commit 5c78912

Browse files
authored
[bug]: Fix wrong order by removal from plan (#13497)
* Initial commit * Fix formatting * Add across partitions check * Add new test case Add a new test case * Fix buggy test
1 parent d9abdad commit 5c78912

File tree

2 files changed

+81
-2
lines changed

2 files changed

+81
-2
lines changed

datafusion/physical-expr/src/equivalence/properties.rs

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -883,9 +883,11 @@ impl EquivalenceProperties {
883883
if self.is_expr_constant(source)
884884
&& !const_exprs_contains(&projected_constants, target)
885885
{
886+
let across_partitions = self.is_expr_constant_accross_partitions(source);
886887
// Expression evaluates to single value
887-
projected_constants
888-
.push(ConstExpr::from(target).with_across_partitions(true));
888+
projected_constants.push(
889+
ConstExpr::from(target).with_across_partitions(across_partitions),
890+
);
889891
}
890892
}
891893
projected_constants
@@ -1014,6 +1016,37 @@ impl EquivalenceProperties {
10141016
is_constant_recurse(&normalized_constants, &normalized_expr)
10151017
}
10161018

1019+
/// This function determines whether the provided expression is constant
1020+
/// across partitions based on the known constants.
1021+
///
1022+
/// # Arguments
1023+
///
1024+
/// - `expr`: A reference to a `Arc<dyn PhysicalExpr>` representing the
1025+
/// expression to be checked.
1026+
///
1027+
/// # Returns
1028+
///
1029+
/// Returns `true` if the expression is constant across all partitions according
1030+
/// to equivalence group, `false` otherwise.
1031+
pub fn is_expr_constant_accross_partitions(
1032+
&self,
1033+
expr: &Arc<dyn PhysicalExpr>,
1034+
) -> bool {
1035+
// As an example, assume that we know columns `a` and `b` are constant.
1036+
// Then, `a`, `b` and `a + b` will all return `true` whereas `c` will
1037+
// return `false`.
1038+
let const_exprs = self.constants.iter().flat_map(|const_expr| {
1039+
if const_expr.across_partitions() {
1040+
Some(Arc::clone(const_expr.expr()))
1041+
} else {
1042+
None
1043+
}
1044+
});
1045+
let normalized_constants = self.eq_group.normalize_exprs(const_exprs);
1046+
let normalized_expr = self.eq_group.normalize_expr(Arc::clone(expr));
1047+
is_constant_recurse(&normalized_constants, &normalized_expr)
1048+
}
1049+
10171050
/// Retrieves the properties for a given physical expression.
10181051
///
10191052
/// This function constructs an [`ExprProperties`] object for the given

datafusion/sqllogictest/test_files/order.slt

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1260,3 +1260,49 @@ limit 2;
12601260

12611261
statement ok
12621262
drop table ordered_table;
1263+
1264+
query TT
1265+
EXPLAIN SELECT
1266+
CASE
1267+
WHEN name = 'name1' THEN 0.0
1268+
WHEN name = 'name2' THEN 0.5
1269+
END AS a
1270+
FROM (
1271+
SELECT 'name1' AS name
1272+
UNION ALL
1273+
SELECT 'name2'
1274+
)
1275+
ORDER BY a DESC;
1276+
----
1277+
logical_plan
1278+
01)Sort: a DESC NULLS FIRST
1279+
02)--Projection: CASE WHEN name = Utf8("name1") THEN Float64(0) WHEN name = Utf8("name2") THEN Float64(0.5) END AS a
1280+
03)----Union
1281+
04)------Projection: Utf8("name1") AS name
1282+
05)--------EmptyRelation
1283+
06)------Projection: Utf8("name2") AS name
1284+
07)--------EmptyRelation
1285+
physical_plan
1286+
01)SortPreservingMergeExec: [a@0 DESC]
1287+
02)--ProjectionExec: expr=[CASE WHEN name@0 = name1 THEN 0 WHEN name@0 = name2 THEN 0.5 END as a]
1288+
03)----UnionExec
1289+
04)------ProjectionExec: expr=[name1 as name]
1290+
05)--------PlaceholderRowExec
1291+
06)------ProjectionExec: expr=[name2 as name]
1292+
07)--------PlaceholderRowExec
1293+
1294+
query R
1295+
SELECT
1296+
CASE
1297+
WHEN name = 'name1' THEN 0.0
1298+
WHEN name = 'name2' THEN 0.5
1299+
END AS a
1300+
FROM (
1301+
SELECT 'name1' AS name
1302+
UNION ALL
1303+
SELECT 'name2'
1304+
)
1305+
ORDER BY a DESC;
1306+
----
1307+
0.5
1308+
0

0 commit comments

Comments
 (0)