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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
