fix(binder): fix function call binding #18177
Conversation
| @@ -19,7 +19,7 @@ use crate::aggregate::AggKind; | |||
| use crate::Result; | |||
|
|
|||
| /// Kind of window functions. | |||
| #[derive(Debug, Display, FromStr, Clone, PartialEq, Eq, Hash)] | |||
| #[derive(Debug, Display, FromStr /* for builtin */, Clone, PartialEq, Eq, Hash)] | |||
There was a problem hiding this comment.
We can only parse builtin window functions directly from a string. For UDAF as window function we have to go through a function look up logic.
| @@ -62,7 +62,7 @@ | |||
| Failed to bind expression: lag(x) | |||
|
|
|||
| Caused by: | |||
| Invalid input syntax: Window function `lag` must have OVER clause | |||
| function lag(integer) does not exist, do you mean log | |||
There was a problem hiding this comment.
Seems this error message is less precise?
There was a problem hiding this comment.
To me, the two error messages both look good. Actually I think it's equally possible that the user 1) forgets to write OVER clause when intended to do a window function call or 2) mistypes the normal scalar function log as lag. I mean from the view of editing distance between the given query and the two correct syntaxes.
There was a problem hiding this comment.
May need additional work to achieve msg like function lag(integer) does not exist as a scalar function, do you mean log or do you forget the OVER clause which is better. We can leave it to future.
| // For type inference, we need to bind the array type first. | ||
| return self.bind_array_transform(f); | ||
| reject_syntax!( |
There was a problem hiding this comment.
Seems heavy to me to introduce a macro instead of just a function to construct
return Err(ErrorCode::InvalidInputSyntax($msg.to_string()).into());
But I'm not going to block this PR with it.
stdrc
force-pushed
the
rc/fix-function-binder
branch
from
0ec8cd5 to
830794c
Compare
There was a problem hiding this comment.
The diff of this file on GitHub is a mess as well. 🤣
Signed-off-by: Richard Chien <[email protected]>
Signed-off-by: Richard Chien <[email protected]>
Signed-off-by: Richard Chien <[email protected]>
Signed-off-by: Richard Chien <[email protected]>
stdrc
force-pushed
the
rc/fix-function-binder
branch
from
830794c to
8574edc
Compare
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Previously our
bind_functionis a mess. An example is #17560, which is because we bind UD(A/T)F and window function in different path, and we never considered a combination of the two.This PR re-organizes
bind_functionand is a pre-requisition for fixing #17560.Checklist
./risedev check(or alias,./risedev c)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.