fix: SanityCheckPlan error with window functions and NVL filter#20231
fix: SanityCheckPlan error with window functions and NVL filter#20231EeshanBembi wants to merge 2 commits intoapache:mainfrom
Conversation
github-actions
bot
added
sqllogictest
EeshanBembi
force-pushed
the
fix/sanity-check-nvl-equivalence-20194
branch
from
5543f1f to
244be39
Compare
|
hi @EeshanBembi |
…he#20194) `collect_columns_from_predicate_inner` was extracting equality pairs where neither side was a Column (e.g. `nvl(col, '0') = '0'`), creating equivalence classes between complex expressions and literals. `normalize_expr`'s deep traversal would then replace the literal inside unrelated sort/window expressions with the complex expression, corrupting the sort ordering and triggering SanityCheckPlan failures. Fix by restricting `collect_columns_from_predicate_inner` to only extract pairs where at least one side is a Column reference, matching the function's documented intent. Also update `extend_constants` to recognize Literal expressions as inherently constant, so constant propagation still works for `complex_expr = literal` predicates. Closes apache#20194
EeshanBembi
force-pushed
the
fix/sanity-check-nvl-equivalence-20194
branch
from
244be39 to
ce9919c
Compare
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Thanks @EeshanBembi for working on this.
| let left_const = | ||
| input_eqs.is_expr_constant(binary.left()).or_else(|| { | ||
| binary | ||
| .left() | ||
| .as_any() | ||
| .downcast_ref::<Literal>() | ||
| .map(|l| AcrossPartitions::Uniform(Some(l.value().clone()))) | ||
| }); | ||
| let right_const = | ||
| input_eqs.is_expr_constant(binary.right()).or_else(|| { |
There was a problem hiding this comment.
Current logic repeats nearly identical is_expr_constant(...).or_else(literal...) blocks for left/right operands.
Could we extract an expr_constant_or_literal(expr, input_eqs) helper here? It would remove duplication and centralize literal/const semantics used by filter equivalence inference.
| // replace those occurrences with the complex expression, corrupting | ||
| // sort orderings. Constant propagation for such pairs is handled | ||
| // separately by `extend_constants`. | ||
| let has_column = binary.left().as_any().downcast_ref::<Column>().is_some() |
There was a problem hiding this comment.
has_column currently means "one side is directly a Column expression," not "expression contains a column."
Would has_direct_column_operand (or similar) be clearer here? It matches the intentional exclusion of complex_expr=literal pairs and avoids misreading this as recursive column detection.
| /// `normalize_expr`'s deep traversal would replace the literal inside | ||
| /// unrelated expressions (e.g. sort keys) with the complex expression. | ||
| #[tokio::test] | ||
| async fn test_collect_columns_skips_non_column_pairs() -> Result<()> { |
There was a problem hiding this comment.
test_collect_columns_skips_non_column_pairs does not use async behavior.
Could this be a plain #[test]? Keeping it sync makes scope a bit clearer since no async execution is involved.
2 participants
Which issue does this PR close?
Closes #20194
Rationale for this change
A query with
ROW_NUMBER() OVER (... ORDER BY CASE WHEN col='0' THEN 1 ELSE 0 END)combined with a filternvl(t2.value_2_3,'0')='0'fails with aSanityCheckPlanerror. This worked in 50.3.0 but broke in 52.1.0.What changes are included in this PR?
Root cause:
collect_columns_from_predicate_innerwas extracting equality pairs where neither side was aColumn(e.g.nvl(col, '0') = '0'), creating equivalence classes between complex expressions and literals.normalize_expr's deep traversal would then replace the literal'0'inside unrelated sort/window CASE WHEN expressions with the complex NVL expression, corrupting the sort ordering and causing a mismatch betweenSortExec's reported output ordering andBoundedWindowAggExec's expected ordering.Fix (two changes in
filter.rs):collect_columns_from_predicate_inner: Only extract equality pairs where at least one side is aColumnreference. This matches the function's documented intent ("Column-Pairs") and prevents complex-expression-to-literal equivalence classes from being created.extend_constants: RecognizeLiteralexpressions as inherently constant (previously only checkedis_expr_constanton the input's equivalence properties, which doesn't know about literals). This ensures constant propagation still works forcomplex_expr = literalpredicates — e.g.nvl(col, '0')is properly marked as constant after the filter.How was this tested?
test_collect_columns_skips_non_column_pairsverifying the filtering logicTest plan
collect_columns_from_predicate_innercolumn filtering