WIP - Remodel aggregate plans #2597
Conversation
Result of fdb-record-layer-pr on Linux CentOS 7
|
| @@ -0,0 +1,12 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
| @Nullable | ||
| private final FieldValue groupingValue; | ||
| @Nonnull | ||
| private final List<Value> groupingValues; |
There was a problem hiding this comment.
So this being a FieldValue was wrong. But this should just be a Value which is a RCV. I mean the result of a SelectExpression is also not a list of Values. This is complicating the expression as you have a bonafide way of expressing a list of expressions by using an RCV, why using the other representation.
|
|
||
| @Nonnull | ||
| private final AggregateValue aggregateValue; | ||
| private final List<AggregateValue> aggregateValues; |
There was a problem hiding this comment.
Some thing here. An RCV is an AggregateValue so it can compose the actual aggregates.
| * @param inner The underlying source of tuples to be grouped. | ||
| */ | ||
| public GroupByExpression(@Nonnull final AggregateValue aggregateValue, | ||
| @Nullable final FieldValue groupingValue, | ||
| public GroupByExpression(@Nonnull final List<AggregateValue> aggregateValues, |
There was a problem hiding this comment.
Why not make another factory method that just takes two lists and than fabricates RCVs both on top of grouping keys as well as aggregate values.
| @@ -91,7 +89,7 @@ public void onMatch(@Nonnull final CascadesRuleCall call) { | |||
|
|
|||
| for (final var planPartition : planPartitions) { | |||
| final var providedOrdering = planPartition.getAttributeValue(OrderingProperty.ORDERING); | |||
| if (requiredOrderingKeyValues == null || providedOrdering.satisfiesGroupingValues(requiredOrderingKeyValues)) { | |||
| if (providedOrdering.satisfiesGroupingValues(requiredOrderingKeyValues)) { | |||
There was a problem hiding this comment.
This may or may not work.
| final var rebasedAggregatedValue = groupByExpression.getAggregateValue().rebase(aliasMap); | ||
| final var rebasedGroupingValue = groupByExpression.getGroupingValue() == null ? null : groupByExpression.getGroupingValue().rebase(aliasMap); | ||
| return RecordQueryStreamingAggregationPlan.ofNested( | ||
| final var rebasedAggregatedValue = RecordConstructorValue.ofUnnamed(groupByExpression.getAggregateValues()).rebase(aliasMap); |
There was a problem hiding this comment.
and now we need to make them RCVs again?
| @@ -256,7 +256,7 @@ public static BindingMatcher<GroupByExpression> groupByExpression(@Nonnull final | |||
| AllOfMatcher.matchingAllOf(GroupByExpression.class, | |||
| ImmutableList.of( | |||
| typedWithDownstream(GroupByExpression.class, | |||
| Extractor.of(GroupByExpression::getAggregateValue, name -> "aggregation(" + name + ")"), | |||
| Extractor.of(GroupByExpression::getAggregateValues, name -> "aggregation(" + name + ")"), | |||
There was a problem hiding this comment.
Is this not used? If it is and the caller is not adapted I would imagine things not working.
No description provided.