-
Notifications
You must be signed in to change notification settings - Fork 312
Surround subquery in the args aggregate function #549
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
Why? This pr is more early than #566. I also add test fully |
@magiskboy like I said there is a conflict in this, so I cannot merge it now. Either you fix it or #566 adds tests. I'll merge whatever happens first. |
pypika/terms.py
Outdated
name=self.name, | ||
args=",".join(self.get_arg_sql(arg, **kwargs) for arg in self.args), | ||
args=",".join( | ||
p.get_sql(with_alias=False, subquery=True, **kwargs) if hasattr(p, "get_sql") else str(p) |
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.
Can you please keep using self.get_arg_sql(arg, **kwargs) for the non subquery case as before? Its implementation isn't exactly equal to str(p).
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 think that this function get_function_sql
is called if once this is a sub-query, but not all of the terms
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.
because function is only used in subquery
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.
Not sure I understood you. I mean changing:
args=",".join( p.get_sql(with_alias=False, subquery=True, **kwargs) if hasattr(p, "get_sql") else str(p)
to:
args=",".join( p.get_sql(with_alias=False, subquery=True, **kwargs) if hasattr(p, "get_sql") else self.get_arg_sql(arg, **kwargs)
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.
what is arg
? I can't see it
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.
You've changed arg to p, as you can see in diff. So that's essentially the element from the self.args, which we are iterating.
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.
Arg is a better variable name in this case btw.
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.
See comments.
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.
Wonderful.
No description provided.