feat: AggregateRel should use grouping references #94
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #94 +/- ##
==========================================
+ Coverage 61.88% 62.21% +0.32%
==========================================
Files 44 44
Lines 9811 9977 +166
==========================================
+ Hits 6072 6207 +135
- Misses 3462 3487 +25
- Partials 277 283 +6 ☔ View full report in Codecov by Sentry. |
plan/builders.go
Outdated
| AggregateColumns(input Rel, measures []AggRelMeasure, groupByCols ...int32) (*AggregateRel, error) | ||
| // Deprecated: Use AggregateExprs(...).Remap() instead. | ||
| AggregateExprsRemap(input Rel, remap []int32, measures []AggRelMeasure, groups ...[]expr.Expression) (*AggregateRel, error) | ||
| // Deprecated: Use Aggregate(...) instead. | ||
| AggregateExprs(input Rel, measures []AggRelMeasure, groups ...[]expr.Expression) (*AggregateRel, error) |
There was a problem hiding this comment.
Optional: Is a Grouping something we could provide a builder for instead of requiring the caller to construct a slice of expression slices?
There was a problem hiding this comment.
I could only think of an API which takes input as grouping Expressions and grouping References and gives the slice of expression slices but that would be same as Aggregate() API. Any other ways to do it?
There was a problem hiding this comment.
If I was using the interface I'd prefer to just call:
func (b *builder) Aggregate(input Rel, measures []AggRelMeasure, groupingExpressions []expr.Expression) (*AggregateRel, error) {
Then Aggregate could either:
- simply assign ascending values to the grouping references for each expression 1:1 (not gaining any benefit of the identical expressions being able to be deduped and other optimization savings)
- assign values to the grouping references for each expression. If an expression duplicates one seen previously then it isn't added to the expression list and a previous grouping reference value is used instead.
The notion of duplication for the second approach could mark equivalence incorrectly. (For instance, perhaps marking a literal 1 as identical to another one even though it is of a different unit.). However I'm not sure it matters.
There was a problem hiding this comment.
@EpsilonPrime Adding to your suggestion a bit. How about we do introduce more API on AggregateRel like below?
func (b *builder) Aggregate(input Rel, measures []AggRelMeasure, groupingExpressions []expr.Expression) (*AggregateRel, error)
func (ar *AggregateRel) addCube(groupingReferences []uint32)
func (ar *AggregateRel) addRollup(groupingReferences []uint32)
func (ar *AggregateRel) addGroupingSet(groupingReferences []uint32)
This decoupling, however, means we are not ensuring AggregateRel always has groupingExpressions of which every expression is always referenced by atleast one groupingReference.
Maybe alternatively use something like
func (b *builder) Aggregate(input Rel, measures []AggRelMeasure, groupingExpressions []expr.Expression) (*AggregateRelBuilder, error)
func (arb *AggregateRelBuilder) addCube(groupingReferences []uint32)
func (arb *AggregateRelBuilder) addRollup(groupingReferences []uint32)
func (arb *AggregateRelBuilder) addGroupingSet(groupingReferences []uint32)
func (arb *AggregateRelBuilder) build() (*AggregateRel, error)
But this is introducing a new builder.
There was a problem hiding this comment.
I do like the idea of adding a builder to ease construction. I would try to avoid having to deal with grouping references unless the builder is going to tell me which reference a given expression is. Perhaps we can return the references as part of adding the expressions?
func (arb *AggregateBuilder) addExpression(expr.Expression) uint32
| }, nil | ||
| } | ||
|
|
||
| func (b *builder) AggregateExprs(input Rel, measures []AggRelMeasure, groups ...[]expr.Expression) (*AggregateRel, error) { | ||
| return b.AggregateExprsRemap(input, nil, measures, groups...) | ||
| } | ||
|
|
||
| func (b *builder) Aggregate(input Rel, measures []AggRelMeasure, groupingExpressions []expr.Expression, groupingReferences [][]uint32) (*AggregateRel, error) { |
There was a problem hiding this comment.
Should this be part of the interface?
| @@ -286,11 +287,13 @@ func (b *builder) AggregateColumnsRemap(input Rel, remap []int32, measures []Agg | |||
| } | |||
| } | |||
|
|
|||
| groupingExpressions, groupingReferences := groupingExprs(exprs) | |||
There was a problem hiding this comment.
We could ditch the Remap versions since they've been deprecated for a while (hehe, for at least two major versions) to avoid needing passing these changes along.
No description provided.