Add SQL exprs needed for custom offset window #1575
Conversation
courtneyholcomb
added
the
Run Tests With Other SQL Engines
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
github-actions
bot
removed
the
Run Tests With Other SQL Engines
courtneyholcomb
force-pushed
the
court/custom-offset1
branch
4 times, most recently
from
4390e63 to
e193228
Compare
There was a problem hiding this comment.
Looks good! I'm not super familiar with using case when exprs, so I want to defer to @plypaul for that. But the other 2 looks good!
|
|
||
|
|
||
| @dataclass(frozen=True, eq=False) | ||
| class SqlArithmeticExpression(SqlExpressionNode): |
There was a problem hiding this comment.
For potentially cleanup, seems like this can replace SqlRatioComputationExpression unless we want to explicitly have a node for that. But if we wanted to do that, i don't see why we wouldn't want to do it for other operations.
There was a problem hiding this comment.
I considered that but decided not to because the ratio expression has a bunch of extra logic related to casting & NULLIF, and I wasn't sure we would always want that for this node!
| parent_nodes += (when,) | ||
| parent_nodes += (then,) |
There was a problem hiding this comment.
nit: don't have to do it
parent_nodes += (when, then)
| ) | ||
|
|
||
| @classmethod | ||
| def id_prefix(cls) -> IdPrefix: # noqa: D102 |
There was a problem hiding this comment.
Not for this PR as we can do a batch cleanup, but we can now use @override to avoid the D102 for many of the implemented methods.
| def bind_parameter_set(self) -> SqlBindParameterSet: # noqa: D102 | ||
| return SqlBindParameterSet() | ||
|
|
||
| def __repr__(self) -> str: # noqa: D105 |
There was a problem hiding this comment.
Not for this PR as we can do a batch cleanup, but this can be moved to a common method in the base class.
| sql=f"DATEADD({granularity.value}, {count_rendered}, {arg_rendered.sql})", | ||
| bind_parameter_set=arg_rendered.bind_parameter_set, | ||
| sql=f"DATEADD({granularity.value}, {count_sql}, {arg_rendered.sql})", | ||
| bind_parameter_set=SqlBindParameterSet.merge_iterable( |
There was a problem hiding this comment.
Doesn't need to be changed, but you can also do arg_rendered.bind_parameter_set.merge(count_rendered.bind_parameter_set)
This includes a CASE expression, an integer expression, and some updates to the add time expression & the window function expression.
Not totally relevant to this PR, but these seem to have been removed somehow.
courtneyholcomb
force-pushed
the
court/custom-offset1
branch
from
618fd1e to
d46303f
Compare
This includes a
CASEexpression, an integer expression, an arithmetic expression, and some updates to the add time expression & window function expressions. These are all needed to build the following SQL column:Note that the first commit is moving the
sql_exprsfile intometricflow-semantics, which is needed for a commit further up the stack.