-
Notifications
You must be signed in to change notification settings - Fork 3
[BSE-4220] Support for converting an Aggregate to SQLGlot #60
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
Conversation
@@ -96,6 +102,17 @@ def mkglot(expressions: list[Expression], _from: Expression, **kwargs) -> Select | |||
return query | |||
|
|||
|
|||
def mkglot_func(op: type[Expression], args: list[Expression]) -> Expression: |
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.
I know it makes code more verbose, but I'd rather have the type stability of passing a list instead of *args.
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.
Fair, but there is a loophole around that:
def mkglot_func(op: type[Expression], *args: list[Expression]) -> Expression:
arguments: list[Expression] = []
for arg in args:
assert isinstance(arg, Expression)
arguments.append(arg)
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.
Some nitpicks, but LGTM.
@@ -96,6 +102,17 @@ def mkglot(expressions: list[Expression], _from: Expression, **kwargs) -> Select | |||
return query | |||
|
|||
|
|||
def mkglot_func(op: type[Expression], args: list[Expression]) -> Expression: |
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.
Fair, but there is a loophole around that:
def mkglot_func(op: type[Expression], *args: list[Expression]) -> Expression:
arguments: list[Expression] = []
for arg in args:
assert isinstance(arg, Expression)
arguments.append(arg)
input=Aggregate( | ||
input=build_simple_scan(), | ||
keys={ | ||
"b": make_relational_column_reference("b"), |
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.
I'm still very much in favor of "let's give this a much shorter name"
Handles safely converting an aggregate to SQLGlot. As with other nodes there are likely missed optimizations to allow fusing select statements.