Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Function call using SelectElements for parameters #28

Open
cam-m opened this issue Jan 25, 2024 · 2 comments
Open

Function call using SelectElements for parameters #28

cam-m opened this issue Jan 25, 2024 · 2 comments

Comments

@cam-m
Copy link
Contributor

cam-m commented Jan 25, 2024

I'm not sure the SimpleSelectStatement.select and PrimaryExpression.FunctionCall rules are correct.

I'm assuming FunctionCall is intended to parse system function calls (E.g. ABS() or DATEADD(), etc)

Problems:

  1. It currently uses the SelectElements rule for its params property;
{infer FunctionCall} function=GlobalReference '(' params=SelectElements? ')' 
      overClause=OverClause?

SelectElements:
    ('ALL'|distinct?='DISTINCT')? elements+=SelectElement (',' elements+=SelectElement)*
;
SelectElement infers SelectElement:
    {infer AllStar} '*'
    | {infer AllTable} variableName=[TableVariableSource:Identifier] '.' '*'
    | {infer ExpressionQuery} expr=Expression ('AS'? name=Identifier)?
;
  1. FunctionCall also directly defines the OVER statement, which should probably only be in SimpleSelectStatement in a window function expression

Happy to propose a fix if I've understood this correctly.

@Lotes
Copy link
Collaborator

Lotes commented Jan 30, 2024

  1. I think there are two kind of functions: Aggregators like SUM/MAX/MIN/COUNT where this rules make sense. And normal functions which I did not have in mind at the time of creating the grammar. Maybe it makes sense to define aggregators hardcoded and do it the right way for normal functions.
  2. The OVER clause has the same thinking behind it. Window functions are only used with aggregators AFAIK.

Side note: You can always allow more than possible and forbid what shall not be possible using a validator. This might help the user to understand differences in language concepts (with nice error messages presumed). But in this case, I think, hardcoded aggregator functions would be better.

@cam-m
Copy link
Contributor Author

cam-m commented Jan 30, 2024

@Lotes Agreed, and i think aggregators only allow a narrow set of args - i think column name or alias.

I understand what you mean re the validator strategy. I see what i can put together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants