-
Notifications
You must be signed in to change notification settings - Fork 1k
feat(snowflake)!: Annotate type for snowflake REGR_SLOPE function #6425
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
base: main
Are you sure you want to change the base?
Conversation
geooo109
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you also check if this function exists in other sql dialects ? (from the docs online)
I assume that by "exists in other sql dialects", you are asking whether it's present elsewhere in SQLGlot - no, it's not. The function itself is supported by some other databases. |
Yeap, I was referring to other databases, not SQLGlot codebase. So, when we introduce a new function expression, the goal is to have a common representation (1 AST node) for all the dialects + add parsing tests ( I did a quick look up and found that spark, databricks, presto, trino, oracle, postgres, and duckdb (you can re-check it also by yourself I maybe missed something) support this function and for all of these your representation is sufficient. So, we can include a |
| exp.MonthsBetween, | ||
| exp.RegrAvgx, | ||
| exp.RegrAvgy, | ||
| exp.RegrSlope, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function returns: If any of the input expressions is of type DECFLOAT, the returned type is DECFLOAT. Otherwise, the returned type is FLOAT.
Currently, SQLGlot doesn't support DECFLOAT , should we add a new type here or use a mapping? @VaggelisD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add support for DECFLOAT, it's different than FLOAT and has variable scale so we probably can't make it a mapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeap agree thanks.
@fivetran-kwoodbeck so let's add a new datatype called DECFLOAT, then we have to make a custom annotation function that first annotates the args of RegrSlope, and based on the types we will output the correct type.
Sounds good. I double checked and also found a few extra, they were all added to the test_dialect.py same as the example you shared. |
Annotate type for snowflake REGR_SLOPE function.
Documentation:
https://docs.snowflake.com/en/sql-reference/functions/regr_slope