-
-
Notifications
You must be signed in to change notification settings - Fork 57
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
Basic classes for Query data model #557
Conversation
3537b0b
to
63086b5
Compare
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.
Haven't had a chance to really digest a lot of this, but here are some initial scattershot thoughts.
The biggest question/confusion I have is around why the distinction between expression and condition is necessary (and I suppose aggregation falls into this category as well.)
Updates:
|
snuba/query/query.py
Outdated
chain(*map(lambda orderby: orderby.node, self.__order_by)), | ||
]) | ||
|
||
def transform_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.
See test_query_ast.py for a concrete example.
snuba/query/query.py
Outdated
@@ -62,6 +96,58 @@ def __init__(self, body: MutableMapping[str, Any], data_source: RelationalSource | |||
self.__data_source = data_source | |||
self.__prewhere_conditions: Sequence[Condition] = [] | |||
|
|||
# New data model | |||
# TODO: Provide a better typing for this. |
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.
Is this comment still accurate? If it is, what specifically is lacking here at this point?
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.
no, will remove
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.
Ok — one more change (sorry): this line was never removed.
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.
Please don't forget about this.
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.
That's weird I was sure I removed it. I guess it must have come back during the merge for black.
Updates:
|
snuba/query/expressions.py
Outdated
parameters_group1: Sequence[Expression] | ||
# This is for expressions like f(x)(y). | ||
# None means there is no second gorup of parameters | ||
parameters_group2: Optional[Sequence[Expression]] = None |
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.
Would it make sense to instead structure this as:
function: Union[str, FunctionCall]
parameters: Sequence[Expression]
… or is this too permissive, since this could lead to calls like f(g(x)(y))(z)
? (Is that even a valid construct? I haven't tested it yet, so I honestly have no idea.)
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 the issue with this is not the example you gave. That one is valid, but it was possible before as well by having one parameter of the first group of f, being a FunctionCall.
The issue is that I could express this:
f(x)(y)(z)(y)
My functional programming enthusiast self speaks now:
My interpretation of this syntax in Clickhouse is that that two sets of parameters are just useless syntactic sugar. But now that you mention it, this may actually a proper currying implementation, in which case it would make sense to consider the external call as an application of a function to the function returned by the application of the internal one.
z = f(x)(y) = f(x) returns g(k) -> z = g(y).
Which means f(x)(y)(z)(w) should be allowed.
My functional programming enthusiast self stops speaking now
In practice my concern with expressing currying properly is that, while it is extremely powerful to evaluate expressions, it may make query processing harder.
I think it is worth giving it a shot anyway, I may just split the FunctionCall implementation, instead of using the Union, this will make it clearer for query processors.
# I need to think over naming better. But this is the idea.
class FunctionCall(ABC):
class BasicFunctionCall(FunctionCall):
name: str
params[...]
class CurriedFunctionCall(FunctionCall):
internal: FunctionCall
params[...]
What do you think?
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 the issue with this is not the example you gave. That one is valid, but it was possible before as well by having one parameter of the first group of f, being a FunctionCall.
Oops — yeah, that was a bad example, but based on the remainder of your comment I think you ended up on the same train of thought that I was.
But now that you mention it, this may actually a proper currying implementation, in which case it would make sense to consider the external call as an application of a function to the function returned by the application of the internal one.
Yeah, this is how I've thought always of it — whether or not that's an accurate reflection of their intent, I'm not sure one way or another.
In practice my concern with expressing currying properly is that, while it is extremely powerful to evaluate expressions, it may make query processing harder.
Do you have any examples why? Nothing sticks out to me immediately but I haven't put much thought into this.
I think it is worth giving it a shot anyway, I may just split the FunctionCall implementation, instead of using the Union, this will make it clearer for query processors.
# I need to think over naming better. But this is the idea. class FunctionCall(ABC): class BasicFunctionCall(FunctionCall): name: str params[...] class CurriedFunctionCall(FunctionCall): internal: FunctionCall params[...]
What do you think?
I think this makes sense in general.
I don't think that we need a shared supertype for these — I'd consider just having FunctionCall(Expression)
and CurriedFunctionCall(Expression)
with the same attributes you described above. I think that not allowing infinitely recursive currying would be an advantage, since it's unclear whether or not ClickHouse allows that (and I don't think we have a use-case for it right now.)
snuba/query/query.py
Outdated
def replace_node(self, new_node: Expression) -> OrderBy: | ||
""" | ||
Returns a new OrderBy clause with a new node. | ||
""" | ||
return OrderBy(self.direction, new_node) |
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 callers use dataclass.replace
instead of this function, or does that have typing implications? Looking at the signature of replace
I could see how some type information would be lost (but I haven't tested this.)
I general I think actually exposing a replace
method here (and on Expression
) that binds the instance argument of replace
to self
would be the nicest API from a caller's perspective. I suspect that this pattern will be used a lot in transform
functions.
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.
For now I relied on dataclasses.replace because, if I defined the replace method in the classes themselves, I did not manage to convince mypy that the types made sense.
Changes:
|
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.
One typo and one question/possible change, otherwise this looks good to me. Thanks for working so hard on this!
snuba/query/expressions.py
Outdated
""" | ||
transformed = replace( | ||
self, | ||
internal=func(self.internal_function), |
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.
internal=func(self.internal_function), | |
internal_function=func(self.internal_function), |
snuba/query/query.py
Outdated
def transform_expressions( | ||
self, | ||
func: Callable[[Expression], Expression], | ||
) -> None: | ||
""" | ||
Transforms in place the current query object by applying a transformation | ||
function to all expressions contained in this query | ||
""" |
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.
Should this return a new Query
to mirror the semantics of transform
?
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 wish , unfortunately we cannot do that just yet:
There are several places during the query execution that rely on Request being a sealed dataclass with a query attribute, thus We cannot swap the Query as of now, since Request is immutable.
This is what drove the QueryProcessor to make changes to the query object in place.
We need first to remove all dependencies on a mutable Query, then we can make Query immutable itself.
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.
That makes sense. It'd probably a good thing to note in a comment that this was a design decision made as a side effect of another design decision (so we don't have to remember why it was written this way if/when we can change it later.)
snuba/query/query.py
Outdated
@@ -62,6 +96,58 @@ def __init__(self, body: MutableMapping[str, Any], data_source: RelationalSource | |||
self.__data_source = data_source | |||
self.__prewhere_conditions: Sequence[Condition] = [] | |||
|
|||
# New data model | |||
# TODO: Provide a better typing for this. |
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.
Please don't forget about this.
The first classes needed to provide the Snuba query class with a proper AST.
This PR represents the Query as a tree. The root is the query. Nodes can be containers
(like selected columns), expressions (simple or nested) or conditions (simple or nested).
The basic idea is to provide three ways to manipulate the Query:
Limitations (will come in followups - see the size of this PR):
Some notes inline, but as an advice for review: