Remove any aliases in Filter::try_new rather than erroring
#11307
Conversation
There was a problem hiding this comment.
Thank you @samuelcolvin
I think the original rationale as I remember for this check was to avoid optimizer passes creating ill formed plans.
I think @andygrove added this check in #3796 and while we can remove it I think there are other options.
You can't trigger this bug in the current sql implementation because it is not syntactically valid to do something like SELECT ... FROM foo WHERE my_user_function(x) as nice_name
However, the user defined planning doesn't have enough context to know if the expr is in the context of a select item (where aliases are allowed) or a filter clause (where aliases are not allowed)
Some other potential options:
- Strip aliases in the sql planner where they don't make sense (as in if an extension planner returns an Alias in WHERE, just remove the alias before creating the filter)
- Add another parameter to
plan_binary_opthat says if aliases are allowed or not (or maybe if it was in a SELECT list)
@andygrove , @jonahgao or @jackwener I wonder if you have any thoughts / opinions?
|
I think aliases in a predicate will not affect functionality. They are just useless because they do not contribute to the output names of the filter plan, nor will any other expressions reference them(with the exception of subqueries). If this is true, we can move the unalias predicate logic to |
|
I've updated this to un-alias the predicate. I'm not sure if this was your suggestion, but I don't think it's necessarily to un-alias recursively within |
| @@ -2130,16 +2130,10 @@ impl Filter { | |||
| } | |||
| } | |||
|
|
|||
| // filter predicates should not be aliased | |||
| if let Expr::Alias(Alias { expr, name, .. }) = predicate { | |||
There was a problem hiding this comment.
I am confused about this check; it is not aligned with unalias_nested.
There was a problem hiding this comment.
I think it is a very old check and substantially predates unalias_nested so it may have become unaligned over time
|
|
||
| Ok(Self { predicate, input }) | ||
| Ok(Self { | ||
| predicate: predicate.unalias_nested().data, |
There was a problem hiding this comment.
My thought is to execute unalias_nested when creating the initial logical plan.
But the current implementation is also acceptable to me. Thank you @samuelcolvin . If we follow the current implementation, we also need to remove the redundant unalias_nested calls outside of try_new.
cc @alamb
|
|
||
| Ok(Self { predicate, input }) | ||
| Ok(Self { | ||
| predicate: predicate.unalias_nested().data, |
There was a problem hiding this comment.
Unaliasing rather than throwing an exception here makes sense to me.
|
Thanks everyone |
Filter::try_new rather than erroring
…11307) * allow alias in predicate, fix apache#11306 * Fix typo. Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> * unalise predicate * use unalias_nested --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
…11307) * allow alias in predicate, fix apache#11306 * Fix typo. Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> * unalise predicate * use unalias_nested --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Which issue does this PR close?
fix #11306.
Rationale for this change
See datafusion-contrib/datafusion-functions-json#26 and #11306
What changes are included in this PR?
Remove the block on aliases as prediates, add test.
Are these changes tested?
Yes, the only way I could get a failure without this change.
Are there any user-facing changes?
AFAIK, we just allow something that should be allowed.
NOTE: Aliases were already allowed in binary expressions, just not as full predicates, so this restriction seemed unusually strict. Also the SQL parser wouldn't allow me to build such an expression directly in SQL, e.g.
from x where (y = z) as the_aliaswas invalid SQL.