korlov42 commented on code in PR #2461:
URL: https://github.com/apache/ignite-3/pull/2461#discussion_r1306958570
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/Commons.java:
##########
@@ -477,7 +477,7 @@ public static <T> Predicate<T> negate(Predicate<T> p) {
* @see org.apache.calcite.plan.RelTrait#apply(TargetMapping)
* @see org.apache.calcite.rel.core.AggregateCall#transform(TargetMapping)
*/
- public static Mappings.TargetMapping trimmingMapping(int sourceSize,
ImmutableBitSet requiredElements) {
+ public static Mapping trimmingMapping(int sourceSize, ImmutableBitSet
requiredElements) {
Mapping mapping = Mappings.create(MappingType.PARTIAL_FUNCTION,
sourceSize, requiredElements.cardinality());
Review Comment:
If you don't mind, let's change `PARTIAL_FUNCTION` to `INVERSE_SURJECTION`.
That way we can get rid of `inverseTrimmingMapping` method, because created
mapping start to work both ways
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItAggregatesTest.java:
##########
@@ -585,6 +585,22 @@ public void testAvgOnEmptyGroup(String[] rules) {
}
}
+ @ParameterizedTest
+ @MethodSource("provideRules")
+ public void testAggDistinctGroupSet(String[] rules) {
+ try {
+ sql("CREATE TABLE testMe (id INTEGER PRIMARY KEY, a INTEGER, b
INTEGER);");
Review Comment:
do we really need to create new table here? Currently, creation of a new
table is a pretty time consuming operation (it takes about 10sec on my laptop).
Does it make sense to use any table from `initTestData`?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rel/agg/IgniteSortAggregateBase.java:
##########
@@ -44,7 +44,34 @@ interface IgniteSortAggregateBase extends
TraitsAwareIgniteRel {
*/
ImmutableBitSet getGroupSet();
- /** {@inheritDoc} */
+ /**
+ * Performs propagation of {@link RelCollation collation trait}.
+ *
+ * <p>Sorted aggregate operate on already sorted input to perform
aggregation/grouping.
+ * Because of that such operator produces rows sorted by grouping columns,
and it can produce
+ * results that may satisfy required collation trait (ordering).
+ *
+ * <p>If a grouping set contains a subset of ordering columns, then inputs
should provide
+ * {@code ordering by grouping set columns + ordering by columns that are
not provided by this operator}:
+ *
+ * <pre>
+ * GROUP BY a, b, c ORDER BY a, b
+ * Input collation: (a, b, c)
+ * </pre>
+ *
+ * <p>Otherwise this operator is unable to fully satisfy required ordering
and we require collation trait based
+ * columns from the grouping set, and the rest of of the requirements is
going to be satisfied by enforcer operator:
Review Comment:
```suggestion
* columns from the grouping set, and the rest of the requirements is
going to be satisfied by enforcer operator:
```
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rule/SortAggregateConverterRule.java:
##########
@@ -143,18 +150,18 @@ public IgniteRel makeReduceAgg(RelOptCluster cluster,
RelNode map, ImmutableBitS
return new IgniteReduceSortAggregate(
cluster,
- outTrait.replace(IgniteDistributions.single()),
- convert(map,
inTrait.replace(IgniteDistributions.single())),
- agg.getGroupSet(),
- agg.getGroupSets(),
+ outTraits.replace(IgniteDistributions.single()),
+ convert(map,
inTraits.replace(IgniteDistributions.single())),
Review Comment:
I would say, input of reduce phase should have the same traits as its output
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rel/agg/IgniteSortAggregateBase.java:
##########
@@ -44,7 +44,34 @@ interface IgniteSortAggregateBase extends
TraitsAwareIgniteRel {
*/
ImmutableBitSet getGroupSet();
- /** {@inheritDoc} */
+ /**
+ * Performs propagation of {@link RelCollation collation trait}.
+ *
+ * <p>Sorted aggregate operate on already sorted input to perform
aggregation/grouping.
+ * Because of that such operator produces rows sorted by grouping columns,
and it can produce
+ * results that may satisfy required collation trait (ordering).
+ *
+ * <p>If a grouping set contains a subset of ordering columns, then inputs
should provide
+ * {@code ordering by grouping set columns + ordering by columns that are
not provided by this operator}:
+ *
+ * <pre>
+ * GROUP BY a, b, c ORDER BY a, b
+ * Input collation: (a, b, c)
+ * </pre>
+ *
+ * <p>Otherwise this operator is unable to fully satisfy required ordering
and we require collation trait based
+ * columns from the grouping set, and the rest of of the requirements is
going to be satisfied by enforcer operator:
+ * <pre>
+ * GROUP BY a, b ORDER BY a, b, c
+ * Input collation (a, b)
+ * Enforcer: adds ordering by c
+ * </pre>
+ *
+ * @param nodeTraits Required relational node output traits.
+ * @param inputTraits Required relational node input traits.
Review Comment:
that is not required traits but actual traits of the input
methods for pass through and derive have similar signature, but different
semantic:
- for pass through we are saying "hey, we want you to satisfy `nodeTraits`.
Btw, here is your input, do you want to adjust it as well?"
- for derive we are saying "hey, let's see what your child is offering to
you (`inputTraits`). Btw, it's you (`nodeTraits`)"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
