[GitHub] [druid] gianm commented on a diff in pull request #13902: Window planning: use collation traits, improve subquery logic.

["gianm (via GitHub)" <gi...@apache.org>]

gianm commented on code in PR #13902:
URL: https://github.com/apache/druid/pull/13902#discussion_r1172185817


##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java:
##########
@@ -1484,16 +1488,16 @@ private ScanQuery toScanQuery()
         // Cannot handle this ordering.
         // Scan cannot ORDER BY non-time columns.
         plannerContext.setPlanningError(
-            "SQL query requires order by non-time column %s that is not supported.",
+            "SQL query requires order by non-time column %s, which is not supported.",
             orderByColumns
         );
         return null;
       }
       if (!dataSource.isConcrete()) {
         // Cannot handle this ordering.
-        // Scan cannot ORDER BY non-time columns.
+        // Scan cannot ORDER BY non-concrete datasources on _any_ column.
         plannerContext.setPlanningError(
-            "SQL query is a scan and requires order by on a datasource[%s], which is not supported.",
+            "SQL query requires order by on non-concrete datasource [%s], which is not supported.",

Review Comment:
   The message was removed anyway in #13965: https://github.com/apache/druid/pull/13965/files#r1145293604.
   
   Although, TBH, I don't understand why it was removed. The comment "Since we are using a table data source and not a query data source now the isConcrete() check is not needed" doesn't really make sense to me. It's still possible for the `dataSource` to be non-concrete at this point in the code. I tried a test query that triggers this case, and the error you get is like this:
   
   ```
   Time-ordering on scan queries is only supported for queries with segment specs of type MultipleSpecificSegmentSpec
   ```
   
   It's from native execution, not from SQL planning, since the query does now pass the SQL planner. IMO in terms of clarity, it's even worse 😛
   
   Test query is:
   
   ```
   select __time as t, m1
   from (select __time, m1 from druid.foo where __time >= timestamp '1970-01-01 00:00:00')
   where (m1 in (select distinct m1 from druid.foo))
   order by 1
   limit 1
   ```
   
   I think we could address this by restoring a check here, but instead of checking `isConcrete`, check `isConcreteBased`. That'd allow joins on concrete stuff, but not subqueries. Then for the message, we could do something like:
   
   ```
   ORDER BY is only supported for __time, and only on regular tables (not subqueries)
   ```
   
   Or, we could spend the time supporting order-by for all scans ✨



-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org