-
Notifications
You must be signed in to change notification settings - Fork 111
Composite aggregates #3266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Composite aggregates #3266
Conversation
a25ae95
to
4c9ec49
Compare
72f7a46
to
2f5e9b8
Compare
8f97639
to
55cd005
Compare
adec3d2
to
91ed1cf
Compare
91ed1cf
to
6fa2c04
Compare
6fa2c04
to
0aeab9c
Compare
@@ -44,6 +44,10 @@ static <T> EnumeratingIterable<T> singleIterable(@Nonnull final T singleElement) | |||
return new SingleIterable<>(singleElement); | |||
} | |||
|
|||
static <T> EnumeratingIterable<T> emptyOnEmptyIterable() { | |||
return new SingleIterable<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the reason for using SingleIterator
(with null
argument) instead of EmptyIterator
is related to returning empty list, compared to returning null
in EmptyIterator
when calling computeNext
, is that so, why not just use EmptyIterator
instead and let the customer deal with the null
?
* @return {@code Optional.empty()} if the match could not be adjusted, Optional.of(matchInfo) for a new adjusted | ||
* match, otherwise. | ||
*/ | ||
@Nonnull | ||
default Optional<MatchInfo> adjustMatch(@Nonnull final PartialMatch partialMatch) { | ||
default Optional<MatchInfo> adjustMatch(@Nonnull final PartialMatch partialMatch, | ||
@Nonnull final Quantifier candidateQuantifier) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this quantifier the one of the child? can we not always infer it considering that this
should have a single child.
@@ -212,7 +212,8 @@ public MatchableSortExpression translateCorrelations(@Nonnull final TranslationM | |||
|
|||
@Nonnull | |||
@Override | |||
public Optional<MatchInfo> adjustMatch(@Nonnull final PartialMatch partialMatch) { | |||
public Optional<MatchInfo> adjustMatch(@Nonnull final PartialMatch partialMatch, | |||
@Nonnull final Quantifier candidateQuantifier) { | |||
final var childMatchInfo = partialMatch.getMatchInfo(); | |||
final var maxMatchMap = childMatchInfo.getMaxMatchMap(); | |||
final var innerQuantifier = Iterables.getOnlyElement(getQuantifiers()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
innerQuantifier
and candidateQuantifier
are always the same I think.
@@ -70,7 +70,7 @@ public class YamlTestExtension implements TestTemplateInvocationContextProvider, | |||
private final boolean includeMethodInDescriptions; | |||
|
|||
public YamlTestExtension() { | |||
this(null, false); | |||
this(null, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this necessary?
- unorderedResult: [{BITMAP: xStartsWith_1250'0200004', 'CATEGORY': 'hello', 'OFFSET':0}, | ||
{BITMAP: xStartsWith_1250'02', 'CATEGORY': 'hello', 'OFFSET':10000}, | ||
{BITMAP: xStartsWith_1250'0400008', 'CATEGORY': 'world', 'OFFSET':0}] | ||
#- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are these tests commented out?
return IntersectionResult.noViableIntersection(); | ||
} | ||
|
||
final var compensation = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we can immediately return IntersectionResult.noViableIntersection()
if the resulting intersection compensation
is impossible, correct?
return RecordConstructorValue.ofColumns(columnBuilder.build()); | ||
} | ||
|
||
private static TranslationMap computeTranslationMap(@Nonnull final CorrelationIdentifier intersectionAlias, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be tagged with @Nonnull
} | ||
isCompensationImpossible |= resultCompensationFunction.isImpossible(); | ||
|
||
groupByMappings = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to add some comments explaining the handling of matched group by aggregations and groups here.
.map(childPlan -> (Function<byte[], RecordCursor<QueryResult>>) | ||
((byte[] childContinuation) -> childPlan | ||
.executePlan(store, context, childContinuation, childExecuteProperties))) | ||
.collect(Collectors.toList()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use ImmutableList
here instead?
} | ||
} | ||
|
||
return IntersectionResult.of(hasCommonOrdering ? intersectionOrdering : null, compensation, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is possible to retrieve an IntersectionResult
which non-empty list of RelationalExpression
s and null
-common ordering, if we examine the code flow above.
Having said that, we could probably cleanup the code, such that we can remove the hasCommonFlag
maybe?
This PR adds the ability to plan arbitrary aggregation queries using aggregation indexes as well as intersections of aggregation indexes. It also corrects/adds the infrastructure to deal with complicated order-by requirements.