Add new dataflow plan nodes for custom offset windows #1579
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
courtneyholcomb
force-pushed
the
court/custom-offset5
branch
from
f267e57 to
57ba604
Compare
courtneyholcomb
force-pushed
the
court/custom-offset4
branch
from
74c5cf7 to
1eb8615
Compare
courtneyholcomb
force-pushed
the
court/custom-offset5
branch
from
57ba604 to
71cef04
Compare
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-offset4
branch
from
1eb8615 to
f24f9fa
Compare
courtneyholcomb
force-pushed
the
court/custom-offset5
branch
2 times, most recently
from
6e642e4 to
a9dc916
Compare
… in FilterElementsNode This will more accurately match the column name since it includes more information about the spec
This will allow us to track which specs have had a window function applied between DataflowPlan nodes
We weren't tracking the parent nodes properly, which resulted in improper optimization and nodes missing when displaying the plan. This should not impact the output data, but will hopefully improve query efficiency now that more CTEs are enabled.
courtneyholcomb
force-pushed
the
court/custom-offset4
branch
from
f24f9fa to
c79153d
Compare
courtneyholcomb
force-pushed
the
court/custom-offset5
branch
from
a9dc916 to
7de2bd3
Compare
plypaul
reviewed
Dec 19, 2024
There was a problem hiding this comment.
Summarizing a chat:
- Combining the two nodes into a single node would be more useful and allow for a more straightforward functional description.
- However, that is blocked on some updates to the optimizer, which I'll make. @courtneyholcomb is planning to refactor once that change is in.
- Rows are not duplicated in the join with the
CASEstatement when computing the offset. However, if the time interval has a different number of days, (e.g. quarters with different days in each), the last day could be an aggregation of multiple days. @courtneyholcomb is verifying that this is inline with product expectations.
Other notes:
What helped during review was an example of the SQL that the nodes would generate along with a few rows that show the respective values, especially for a boundary.
Could you include a test that includes those using a minimal DAG? pytest.mark.duckdb_only makes sense to use given that more extensive tests are present later in the stack.
| @@ -56,6 +56,8 @@ class StaticIdPrefix(IdPrefix, Enum, metaclass=EnumMetaClassHelper): | |||
| DATAFLOW_NODE_JOIN_CONVERSION_EVENTS_PREFIX = "jce" | |||
| DATAFLOW_NODE_WINDOW_REAGGREGATION_ID_PREFIX = "wr" | |||
| DATAFLOW_NODE_ALIAS_SPECS_ID_PREFIX = "as" | |||
| DATAFLOW_NODE_CUSTOM_GRANULARITY_BOUNDS_ID_PREFIX = "cgb" | |||
| DATAFLOW_NODE_OFFSET_BY_CUSTOMG_GRANULARITY_ID_PREFIX = "obcg" | |||
| custom_granularity_bounds_node: Optional[CustomGranularityBoundsNode] = None | ||
| filter_elements_node: Optional[FilterElementsNode] = None | ||
| for parent_node in new_parent_nodes: | ||
| if isinstance(parent_node, CustomGranularityBoundsNode): |
There was a problem hiding this comment.
The combined node should allow this to be removed.
courtneyholcomb
force-pushed
the
court/custom-offset4
branch
from
c79153d to
147c9d2
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds two new nodes -
CustomGranularityBoundsNodeandOffsetByCustomGranularityNode. These will be used to build the SQL for custom offset windows.