Skip to content

[CALCITE-3448] AggregateOnCalcToAggregateUnifyRule ignores Project…#1532

Closed
jinxing64 wants to merge 3 commits intoapache:masterfrom
jinxing64:CALCITE-3448
Closed

[CALCITE-3448] AggregateOnCalcToAggregateUnifyRule ignores Project…#1532
jinxing64 wants to merge 3 commits intoapache:masterfrom
jinxing64:CALCITE-3448

Conversation

@jinxing64
Copy link
Contributor

@jinxing64 jinxing64 commented Oct 25, 2019 •
edited
Loading

… incorrectly when there's missing grouping or mapping breaks ordering (Jin Xing)

CALCITE-3087 fixed AggregateOnProjectToAggregateUnifyRule when mapping under Aggregate breaks ordering. But it fails when there's missing grouping in query compared with target.
Below matching fails:

        MV:
        select empid, deptno, name, count(*) from emps
        group by empid, deptno, name

        Query:
        select name, empid, count(*) from emps group by name, empid

Note that -- comparing groupings in MV (empid, deptno, name) and groupings in query (name, empid), deptno is missed and ordering is broken

@jinxing64 jinxing64 force-pushed the CALCITE-3448 branch 2 times, most recently from ab88eb4 to 9dfb1b1 Compare October 25, 2019 14:13
@jinxing64
Copy link
Contributor Author

jinxing64 commented Oct 26, 2019

cc @DonnyZone
I touched your code. Please give a check when you have time :)
Thanks a lot ~

@DonnyZone
Copy link
Contributor

DonnyZone commented Oct 28, 2019

Hi @jinxing64, sorry for the late reply! I'm outing these days, and I will take a look tomorrow.

@jinxing64
Copy link
Contributor Author

jinxing64 commented Oct 28, 2019

Thanks a lot @DonnyZone ~

DonnyZone
DonnyZone reviewed Oct 28, 2019
View reviewed changes
core/src/main/java/org/apache/calcite/util/mapping/Mappings.java
return false;
}
prevTarget = target;
if (target != -1) {
Copy link
Contributor

@DonnyZone DonnyZone Oct 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can combine two if clause together?

Copy link
Contributor Author

@jinxing64 jinxing64 Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

@DonnyZone
Copy link
Contributor

DonnyZone commented Oct 28, 2019

LGTM!

@DonnyZone
Copy link
Contributor

DonnyZone commented Oct 28, 2019

By the way, have you test the below case?

Query:
select name, name, count(*) from emps group by name, empid

@jinxing64
Copy link
Contributor Author

jinxing64 commented Oct 29, 2019 •
edited
Loading

I tested the test case you provided @DonnyZone

MV:
select empid, deptno, name, count(*) from emps
group by empid, deptno, name
Query:
select name, name, count(*) from emps group by name, empid

The result is correct as below:

EnumerableCalc(expr#0..2=[{inputs}], name=[$t1], name0=[$t1], EXPR$1=[$t2])
   EnumerableAggregate(group=[{0, 2}], EXPR$2=[$SUM0($3)])
     EnumerableTableScan(table=[[hr, m0]])

@jinxing64
Copy link
Contributor Author

jinxing64 commented Oct 29, 2019

Hi Zhu Feng ~
Thanks a lot for your review and approval and I updated the PR by your comments

@jinxing64 jinxing64 changed the title [CALCITE-3448] AggregateOnProjectToAggregateUnifyRule ignores Project… [CALCITE-3448] AggregateOnCalcToAggregateUnifyRule ignores Project… Oct 30, 2019
@jinxing64
[CALCITE-3448] AggregateOnCalcToAggregateUnifyRule ignores Project in…
f9da979 
…correctly when there's missing grouping or mapping breaks ordering (Jin Xing)
@jinxing64
Refine by DonnyZone's comment
c7a3db2
@jinxing64
Copy link
Contributor Author

jinxing64 commented Nov 1, 2019

Hi ~ committers,
The bug this PR tries to fix seems obvious ~
Could you please help review this ?
It's great if I can get your comments and merge it if possible ~

@DonnyZone
Copy link
Contributor

DonnyZone commented Nov 1, 2019

Ping @hsyuan

@hsyuan
Copy link
Member

hsyuan commented Nov 2, 2019

Will do

hsyuan
hsyuan reviewed Nov 5, 2019
View reviewed changes
hsyuan
hsyuan approved these changes Nov 12, 2019
View reviewed changes
Copy link
Member

@hsyuan hsyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@hsyuan hsyuan added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Nov 12, 2019
@jinxing64
Copy link
Contributor Author

jinxing64 commented Nov 12, 2019

Thanks a lot for approval ~

@hsyuan hsyuan closed this in 3853118 Nov 12, 2019
@jinxing64 jinxing64 deleted the CALCITE-3448 branch November 13, 2019 02:39
XuQianJin-Stars pushed a commit to XuQianJin-Stars/calcite that referenced this pull request Nov 17, 2019
@jinxing64 @XuQianJin-Stars
[CALCITE-3448] AggregateOnCalcToAggUnifyRule may ignore Project incor…
528bf96 
…rectly (Jin Xing)

CALCITE-3087 fixed AggregateOnProjectToAggregateUnifyRule when mapping under
Aggregate breaks ordering. But it fails when there's missing grouping in query
compared with target.

Below matching fails:
 MV:
 select empid, deptno, name, count(*) from emps
 group by empid, deptno, name

 Query:
 select name, empid, count(*) from emps group by name, empid

Note that -- comparing groupings in MV (empid, deptno, name) and groupings in
query (name, empid), deptno is missed and ordering is broken

Close apache#1532
XuQianJin-Stars pushed a commit to XuQianJin-Stars/calcite that referenced this pull request Nov 20, 2019
@jinxing64 @XuQianJin-Stars
[CALCITE-3448] AggregateOnCalcToAggUnifyRule may ignore Project incor…
bfaa961 
…rectly (Jin Xing)

CALCITE-3087 fixed AggregateOnProjectToAggregateUnifyRule when mapping under
Aggregate breaks ordering. But it fails when there's missing grouping in query
compared with target.

Below matching fails:
 MV:
 select empid, deptno, name, count(*) from emps
 group by empid, deptno, name

 Query:
 select name, empid, count(*) from emps group by name, empid

Note that -- comparing groupings in MV (empid, deptno, name) and groupings in
query (name, empid), deptno is missed and ordering is broken

Close apache#1532
XuQianJin-Stars pushed a commit to XuQianJin-Stars/calcite that referenced this pull request Nov 20, 2019
@jinxing64 @XuQianJin-Stars
[CALCITE-3448] AggregateOnCalcToAggUnifyRule may ignore Project incor…
d325aae 
…rectly (Jin Xing)

CALCITE-3087 fixed AggregateOnProjectToAggregateUnifyRule when mapping under
Aggregate breaks ordering. But it fails when there's missing grouping in query
compared with target.

Below matching fails:
 MV:
 select empid, deptno, name, count(*) from emps
 group by empid, deptno, name

 Query:
 select name, empid, count(*) from emps group by name, empid

Note that -- comparing groupings in MV (empid, deptno, name) and groupings in
query (name, empid), deptno is missed and ordering is broken

Close apache#1532
wangxlong pushed a commit to wangxlong/calcite that referenced this pull request Feb 13, 2020
@jinxing64 @hsyuan
[CALCITE-3448] AggregateOnCalcToAggUnifyRule may ignore Project incor…
c35abf3 
…rectly (Jin Xing)

CALCITE-3087 fixed AggregateOnProjectToAggregateUnifyRule when mapping under
Aggregate breaks ordering. But it fails when there's missing grouping in query
compared with target.

Below matching fails:
 MV:
 select empid, deptno, name, count(*) from emps
 group by empid, deptno, name

 Query:
 select name, empid, count(*) from emps group by name, empid

Note that -- comparing groupings in MV (empid, deptno, name) and groupings in
query (name, empid), deptno is missed and ordering is broken

Close apache#1532
jamesstarr pushed a commit to jamesstarr/calcite that referenced this pull request Aug 28, 2025
@yueming-dremio @jinxing64
[CALCITE-3448] AggregateOnCalcToAggUnifyRule may ignore Project incor… (
56aa025 
apache#35)

…rectly (Jin Xing)

CALCITE-3087 fixed AggregateOnProjectToAggregateUnifyRule when mapping
under Aggregate breaks ordering. But it fails when there's missing
grouping in query compared with target.

Below matching fails:
 MV:
 select empid, deptno, name, count(*) from emps
 group by empid, deptno, name

 Query:
 select name, empid, count(*) from emps group by name, empid

Note that -- comparing groupings in MV (empid, deptno, name) and
groupings in query (name, empid), deptno is missed and ordering is
broken

Close apache#1532

Change-Id: I4a8086d0181774e618237374d73e6a5b062f3b37

Co-authored-by: jinxing <jinxing.corey@gmail.com>
jamesstarr pushed a commit to jamesstarr/calcite that referenced this pull request Mar 16, 2026
@yueming-dremio @jinxing64 @jamesstarr
[CALCITE-3448] AggregateOnCalcToAggUnifyRule may ignore Project incor… (
14808e9 
apache#35)

…rectly (Jin Xing)

CALCITE-3087 fixed AggregateOnProjectToAggregateUnifyRule when mapping
under Aggregate breaks ordering. But it fails when there's missing
grouping in query compared with target.

Below matching fails:
 MV:
 select empid, deptno, name, count(*) from emps
 group by empid, deptno, name

 Query:
 select name, empid, count(*) from emps group by name, empid

Note that -- comparing groupings in MV (empid, deptno, name) and
groupings in query (name, empid), deptno is missed and ordering is
broken

Close apache#1532

Change-Id: I4a8086d0181774e618237374d73e6a5b062f3b37

Co-authored-by: jinxing <jinxing.corey@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DonnyZone DonnyZone DonnyZone left review comments

@hsyuan hsyuan hsyuan approved these changes

Assignees

No one assigned

Labels

LGTM-will-merge-soon Overall PR looks OK. Only minor things left.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jinxing64 @DonnyZone @hsyuan