Hi Julian, Since Milinda has already created CALCITE-822, and submitted a patch, it would makes sense to use his patch.
I'll submit a separate patch to add the query as a test case. On Fri, Aug 7, 2015 at 7:23 AM, Milinda Pathirage <[email protected]> wrote: > Hi Julian, > > I created an issue [1] while ago and also attached a patch. > > Thanks > Milinda > > [1] https://issues.apache.org/jira/browse/CALCITE-822 > > On Thu, Aug 6, 2015 at 9:21 PM, Julian Hyde <[email protected]> wrote: > > > If it causes a regression in Drill then I think we should back it out. > > Especially as we have a week to stabilize the release. > > > > Can you submit two commits, one backing out the fix to 783, and one > > adding the above query as a test case. I don't recall whether there is > > a jira case for the issue caused by the "fix" to 783, but it might > > make sense to create one. > > > > Julian > > > > > > On Thu, Aug 6, 2015 at 5:05 PM, Jinfeng Ni <[email protected]> > wrote: > > > Hi Julian, > > > > > > Are we going to fix the bug introduced in CALCITE-783 [1] ( > > > https://issues.apache.org/jira/browse/CALCITE-783), regarding the > > collation > > > for LogicalAggregate? The code[2] is not right, by simply using > input's > > > collation when construct a LogicalAggregate. > > > > > > This issue causes couple of regression test cases failure in Drill > after > > we > > > re-base Drill on top of Calcite-1.4.0-SNAPSHOT, as LogicalAggregate > does > > > not have valid collation. > > > > > > Also, this issue actually will cause the following query to hit > > CanNotPlan > > > exception in Calcite master branch (executed in Calcite sqlline): > > > > > > select sum(x), > > > count(distinct y) > > > from > > > ( > > > select > > > count(*) as x, > > > t1.JOB as y, > > > t1.DEPTNO as z > > > from > > > SCOTT.EMP t1 > > > group by t1.JOB, t1.DEPTNO > > > order by t1.JOB, t1.DEPTNO > > > ) sq(x,y,z) > > > group by z; > > > > > > Node [rel#37:Subset#7.ENUMERABLE.[]] could not be implemented; planner > > > state: > > > .... > > > > > > If I reverse the change of LogicalAggregate made in CALCITE-783, then > the > > > query would run successful. > > > > > > I just want to know whether we are going to address this bug in release > > > 1.4.0, or the next release. > > > > > > Regards, > > > Jinfeng > > > > > > > > > [1] > > > > > > https://github.com/apache/incubator-calcite/commit/c711fed6e2392f296554719a98b2d737da53c9b5 > > > [2] > > > > > > https://github.com/apache/incubator-calcite/blob/master/core/src/main/java/org/apache/calcite/rel/logical/LogicalAggregate.java#L99-L111 > > > > > > > > > > > > On Sat, Aug 1, 2015 at 8:23 PM, Jacques Nadeau <[email protected]> > > wrote: > > > > > >> :D > > >> > > >> On Sat, Aug 1, 2015 at 7:02 PM, Julian Hyde <[email protected]> > > >> wrote: > > >> > > >> > Careful with making the database adhere to people’s “expectations". > > MySQL > > >> > used to guarantee that the output of GROUP BY is sorted and they > > lived to > > >> > regret it. > > >> > > > >> > > On Aug 1, 2015, at 8:58 AM, Jacques Nadeau <[email protected]> > > wrote: > > >> > > > > >> > > I think Aman and Vicki mentioned that this is a situation where > most > > >> > > databases diverge from the spec since people have a certain > > >> expectation. > > >> > > > > >> > > On Fri, Jul 31, 2015 at 9:56 PM, Julian Hyde <[email protected]> > > wrote: > > >> > > > > >> > >> > > >> > >>> Jinfeng, the subquery's ORDER-BY can be dropped in some cases > but > > not > > >> > >> all.. > > >> > >>> for instance in the following query: > > >> > >>> SELECT a1 FROM (SELECT a1 FROM t1 WHERE .... ORDER BY a1) > LIMIT > > 10; > > >> > >>> The OB should not be dropped. There are other cases, this is > one > > >> > >> example. > > >> > >> > > >> > >> FWIW, My understanding of the SQL standard is that that ORDER BY > > can > > >> be > > >> > >> dropped. You can’t rely on the order of output from a sub-query. > If > > >> you > > >> > >> want the desired effect you have to combine ORDER BY and LIMIT > into > > >> the > > >> > >> same clause. > > >> > >> > > >> > >> Julian > > >> > >> > > >> > >> > > >> > > > >> > > > >> > > > > > > -- > Milinda Pathirage > > PhD Student | Research Assistant > School of Informatics and Computing | Data to Insight Center > Indiana University > > twitter: milindalakmal > skype: milinda.pathirage > blog: http://milinda.pathirage.org >
