IGNITE-20164 Sql. Incorrect propagation of RelCollation trait for Sort-based map/reduce aggregates by lowka · Pull Request #2461 · apache/ignite-3

lowka · 2023-08-17T15:08:04Z

Updates grouping set columns on REDUCE phase of a sort aggregate to fix collation propagation.

Updates some javadocs describing trait propagation.
Migrates code that creates mapping for MAP/REDUCE aggregates to Commons::trimmingMapping.
Updates Commons::trimmingMapping to use MappingType.INVERSE_SURJECTION to make usage of Commons::inverseTrimmingMapping redundant because trimmingMapping::getSource returns the same values.

https://issues.apache.org/jira/browse/IGNITE-20164

Thank you for submitting the pull request.

To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:

The Review Checklist

Formal criteria: TC status, codestyle, mandatory documentation. Also make sure to complete the following:
- There is a single JIRA ticket related to the pull request.
- The web-link to the pull request is attached to the JIRA ticket.
- The JIRA ticket has the Patch Available state.
- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
Design: new code conforms with the design principles of the components it is added to.
Patch quality: patch cannot be split into smaller pieces, its size must be reasonable.
Code quality: code is clean and readable, necessary developer documentation is added if needed.
Tests code quality: test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.

Notes

Apache Ignite Coding Guidelines

lowka · 2023-08-22T10:40:14Z

modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/Commons.java

     * @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) {


Mappings.apply(mapping, ImmutableBitSet) accepts Mapping instead of Mappings.TargetMapping.

lowka · 2023-08-22T13:02:22Z

modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/Commons.java

     * @see #trimmingMapping(int, ImmutableBitSet)
     */
-    public static Mappings.TargetMapping inverseTrimmingMapping(int sourceSize, ImmutableBitSet requiredElements) {
+    public static Mapping inverseTrimmingMapping(int sourceSize, ImmutableBitSet requiredElements) {


To have access to getSource in PlanUtils::createHashAggRowType

korlov42 · 2023-08-28T06:22:23Z

modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/Commons.java

     */
-    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());


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

korlov42 · 2023-08-28T06:32:34Z

.../runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItAggregatesTest.java

+    @MethodSource("provideRules")
+    public void testAggDistinctGroupSet(String[] rules) {
+        try {
+            sql("CREATE TABLE testMe (id INTEGER PRIMARY KEY, a INTEGER, b INTEGER);");


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?

Moved all CREATE TABLE statements in this file to initTestData.

korlov42 · 2023-08-28T06:48:38Z

...ine/src/main/java/org/apache/ignite/internal/sql/engine/rel/agg/IgniteSortAggregateBase.java

+     * </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:


Suggested change

* columns from the grouping set, and the rest of of the requirements is going to be satisfied by enforcer operator:

* columns from the grouping set, and the rest of the requirements is going to be satisfied by enforcer operator:

korlov42 · 2023-08-28T06:59:57Z

...ine/src/main/java/org/apache/ignite/internal/sql/engine/rel/agg/IgniteSortAggregateBase.java

+     * </pre>
+     *
+     * @param nodeTraits Required relational node output traits.
+     * @param inputTraits Required relational node input traits.


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)"

korlov42 · 2023-08-28T07:28:37Z

...ine/src/main/java/org/apache/ignite/internal/sql/engine/rule/SortAggregateConverterRule.java

-                            agg.getGroupSet(),
-                            agg.getGroupSets(),
+                            outTraits.replace(IgniteDistributions.single()),
+                            convert(map, inTraits.replace(IgniteDistributions.single())),


I would say, input of reduce phase should have the same traits as its output

AMashenkov · 2023-08-28T12:59:46Z

...rc/test/java/org/apache/ignite/internal/sql/engine/planner/AbstractAggregatePlannerTest.java

+         *
+         * <p>Distribution hash(0)
+         */
+        CASE_24_1A("SELECT COUNT(val0), COUNT(DISTINCT(val1)) from test", schema(hash()))


Let's also check plans for hash distribution: hash(val1)

Added test cases for both hash(val0) and hash(val1).

…rt-based map/reduce aggregates

…rt-based map/reduce aggregates Aggregate planner tests: Add test cases for identity distribution.

…rt-based map/reduce aggregates Fix checkstyle issues.

lowka force-pushed the IGNITE-20164 branch 8 times, most recently from 79208fe to f2285cc Compare August 22, 2023 10:38

lowka commented Aug 22, 2023

View reviewed changes

lowka force-pushed the IGNITE-20164 branch from f2285cc to 9945fa4 Compare August 22, 2023 13:00

lowka commented Aug 22, 2023

View reviewed changes

lowka force-pushed the IGNITE-20164 branch 5 times, most recently from 5f26578 to 636730b Compare August 22, 2023 14:01

lowka marked this pull request as ready for review August 22, 2023 14:57

lowka force-pushed the IGNITE-20164 branch from 636730b to 8c3c805 Compare August 22, 2023 15:03

lowka changed the title ~~[IGNITE-20164] Sql. Incorrect propagation of RelCollation trait for Sort-based map/reduce aggregates.~~ IGNITE-20164 Sql. Incorrect propagation of RelCollation trait for Sort-based map/reduce aggregates. Aug 23, 2023

lowka changed the title ~~IGNITE-20164 Sql. Incorrect propagation of RelCollation trait for Sort-based map/reduce aggregates.~~ IGNITE-20164 Sql. Incorrect propagation of RelCollation trait for Sort-based map/reduce aggregates Aug 23, 2023

korlov42 reviewed Aug 28, 2023

View reviewed changes

AMashenkov reviewed Aug 28, 2023

View reviewed changes

lowka requested review from AMashenkov and korlov42 August 29, 2023 05:47

korlov42 approved these changes Aug 30, 2023

View reviewed changes

zstan approved these changes Aug 31, 2023

View reviewed changes

IGNITE-20164: Sql. Incorrect propagation of RelCollation trait for So…

c99529c

…rt-based map/reduce aggregates

lowka force-pushed the IGNITE-20164 branch from c52d9d4 to c99529c Compare August 31, 2023 13:29

lowka added 2 commits August 31, 2023 16:40

IGNITE-20164: Sql. Incorrect propagation of RelCollation trait for So…

8cd0210

…rt-based map/reduce aggregates Aggregate planner tests: Add test cases for identity distribution.

IGNITE-20164: Sql. Incorrect propagation of RelCollation trait for So…

92c06d8

…rt-based map/reduce aggregates Fix checkstyle issues.

lowka requested review from korlov42 and zstan September 1, 2023 08:34

zstan approved these changes Sep 1, 2023

View reviewed changes

korlov42 merged commit 950a3c0 into apache:main Sep 1, 2023

korlov42 deleted the IGNITE-20164 branch September 1, 2023 11:08

	* columns from the grouping set, and the rest of of the requirements is going to be satisfied by enforcer operator:
	* columns from the grouping set, and the rest of the requirements is going to be satisfied by enforcer operator:

Conversation

lowka commented Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The Review Checklist

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lowka commented Aug 17, 2023 •

edited

Loading