feat: Aggregate filter alias targeting #3252
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3252 +/- ##
===========================================
+ Coverage 78.06% 78.06% +0.01%
===========================================
Files 382 382
Lines 35364 35371 +7
===========================================
+ Hits 27604 27612 +8
Misses 6123 6123
+ Partials 1637 1636 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 17 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -102,7 +106,7 @@ func (n *averageNode) Next() (bool, error) { | |||
| return false, client.NewErrUnhandledType("sum", sumProp) | |||
| } | |||
|
|
|||
| return true, nil | |||
| return mapper.RunFilter(n.currentValue, n.aggregateFilter) | |||
There was a problem hiding this comment.
question: Why are you running the filter in the aggregate nodes, and not the select nodes they sit on top of (that also run the aggregate filters)?
There was a problem hiding this comment.
Or, is this more like a scan level filter? And it is filtering the value produced by the aggregate node? In which case would it not be better to run the filter in the select node that sits on top of the aggregates, allowing the aggregate nodes to remain focused on aggregating, and not adding an additional code-location in which filters may execute?
Or perhaps we could argue that the select node is doing too much, and we should in fact remove filtering from the Select node's remit, and instead introduce a filter node, removing filtering from select and aggregate nodes (and anywhere else in the planner that filters may have leaked into).
EDIT: Sorry for the notification spam, please consider extracting this and the select-filters to now to be a 'todo'
There was a problem hiding this comment.
I'm on board with moving it into a new Filter node but not within this same PR.
There was a problem hiding this comment.
What is the rush on this feature? IMO refactors like this should be done before the feature, not after (or, worst case, during and then broken out to merge first). We need to think about the long term implications of how we build features before we build them.
The tech. debt here is basically the entirety of the production code change - there appears to be very little cost in delaying it and doing it in a way that doesn't build up tech. debt.
There was a problem hiding this comment.
There's no rush. I'm trying to keep my PRs more focused, so I'll be doing the refactor in a different PR.
There was a problem hiding this comment.
Okay, please merge that PR before you merge this one.
Happy to chat about this with the team over discord, we dont have to wait until the next standup.
nasdf
dismissed
AndrewSisley’s stale review
will revisit filter alias refactor in the future
|
bug bash result: |
|
this one is probably doesn't not pertain to the task, but found during the bug bash |
Relevant issue(s)
Resolves #3195
Description
This PR enables aggregate alias targeting in filters.
Blocked by #3253
Tasks
How has this been tested?
Added and updated integration tests.
Specify the platform(s) on which this was tested: