It makes sense to me to get everything fully specified at planning time. I agree with you that the proposed change would not impact the Drill's execution, since Drill currently already uses "nulls high".
The only concern I have is about JDBC data source that Drill is pushing to. If we get the sort order fully specified in planning time, then the same query might see different results when it goes through the native engine (which uses a different sort order), or it goes through Drill. On Mon, Nov 16, 2015 at 2:38 PM, Julian Hyde <[email protected]> wrote: > Currently the null direction is resolved at execution time. As you noted, you > get an UNSPECIFIED in LogicalSortRel. > > After 970, the null direction is resolved at sql-to-rel time. This would > solve several problems, including the problem that different engines (say > Drill and a JDBC data source that Drill is pushing to) might have a different > sort order. In my opinion it is better if, at planning time, everything is > fully specified. > > It sounds as if Drill is already using the proposed “nulls high” policy, so I > don’t think there would be any change in the behavior as seen by end users. > > I tried to minimize changes to EXPLAIN. I changed the > RelFieldCollation.toString() method (and some other places) so that “1 asc > nulls last” prints as “1” (previously it was “1 asc unspecified” that printed > as “1”). You should not have to make many changes to expected test output, > even though the contents of RelFieldCollation have changed. > > Julian > > >> On Nov 16, 2015, at 1:39 PM, Jinfeng Ni <[email protected]> wrote: >> >> Julian, >> >> I'm trying to see if CALCITE-969/970 may have any impact on Drilll side. >> >> I assume that the following logic happens in Calcite's execution side, right? >> " Currently >> >> ORDER BY x DESC >> >> is equivalent to >> >> ORDER BY x DESC NULLS LAST >> " >> >> On 1.4.0, for "ORDER BY x DECS", I saw the nullDirection is >> "UNSPECIFIED" in LogicalSortRel. I did not see "NULLS LAST" added to >> "ORDER BY x DESC" in Calcite logical world. >> >> Drill's execution would interpret "UNSPECIFIED" as "NULL FIRST for >> DESC" and "NULL last for ASC" [1]. So, I assume the thing you talked >> above is also happening in Calcite's execution time. That's, you will >> still keep "UNSPECIFIED" in LogicalSort, if user does not specify >> nullDirection, and leave each different execution engine to choose >> different interpretation. >> >> Thanks, >> >> Jinfeng >> >> >> [1]. >> https://github.com/apache/drill/blob/master/logical/src/main/java/org/apache/drill/common/logical/data/Order.java#L195-L202 >> >> >> >> >> >> >> >> On Mon, Nov 16, 2015 at 11:50 AM, Julian Hyde <[email protected]> wrote: >>> >>> >>>> On Nov 15, 2015, at 6:50 PM, Li Yang <[email protected]> wrote: >>>> >>>> https://issues.apache.org/jira/browse/CALCITE-969 >>> >>> Li Yang, Thanks for logging that. >>> >>> I realized that we have two issues, so I logged >>> https://issues.apache.org/jira/browse/CALCITE-970 for the other part. 970 >>> makes NULL values sort HIGH, by default, which is consistent with Oracle. >>> >>> But it will change some SQL behavior. Currently >>> >>> ORDER BY x DESC >>> >>> is equivalent to >>> >>> ORDER BY x DESC NULLS LAST >>> >>> but after 970 it will be equivalent to >>> >>> ORDER BY x DESC NULLS FIRST >>> >>> If you like the current behavior, you can set defaultNullCollation=LAST. >>> Also, I tightened up what happens in RelNode land; we now use >>> NullDirection.UNSPECIFIED a lot less than we used to. >>> >>> Please holler if you don’t like 970. >>> >>> Julian >>> >
