clintropolis commented on code in PR #14007:
URL: https://github.com/apache/druid/pull/14007#discussion_r1163940181
##########
sql/src/test/resources/calcite/tests/window/wikipediaAggregationsMultipleOrderingDesc.sqlTest:
##########
@@ -10,16 +10,22 @@ sql: |
FROM wikipedia
GROUP BY 1, 2
ORDER BY 1 DESC, 2 DESC
+ LIMIT 2
expectedOperators:
+ - {type: "naiveSort", columns: [{column: "d0", direction: "ASC"}, {column:
"d1", direction: "DESC"}]}
- { type: "naivePartition", partitionColumns: [ "d0" ] }
- type: "window"
processor:
type: "framedAgg"
frame: { peerType: "ROWS", lowUnbounded: false, lowOffset: 3,
uppUnbounded: false, uppOffset: 2 }
aggregations:
- { type: "longSum", name: "w0", fieldName: "a0" }
- - { type: "naiveSort", columns: [ { column: "d1", direction: "DESC" }, {
column: "a0", direction: "DESC"} ]}
Review Comment:
any findings?
##########
sql/src/test/resources/calcite/tests/window/wikipediaAggregationsMultipleOrderingDesc.sqlTest:
##########
@@ -10,16 +10,22 @@ sql: |
FROM wikipedia
GROUP BY 1, 2
ORDER BY 1 DESC, 2 DESC
+ LIMIT 2
expectedOperators:
+ - {type: "naiveSort", columns: [{column: "d0", direction: "ASC"}, {column:
"d1", direction: "DESC"}]}
Review Comment:
why is d0 asc? it looks like the query is specifying desc for both dims
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidOperatorTable.java:
##########
@@ -340,6 +340,7 @@ public class DruidOperatorTable implements SqlOperatorTable
.add(new
AliasedOperatorConversion(CHARACTER_LENGTH_CONVERSION, "LENGTH"))
.add(new
AliasedOperatorConversion(CHARACTER_LENGTH_CONVERSION, "STRLEN"))
.add(new
DirectOperatorConversion(SqlStdOperatorTable.CONCAT, "concat"))
+ .add(new DirectOperatorConversion(SqlStdOperatorTable.DESC,
"desc"))
Review Comment:
a `DirectOperatorConversion` doesn't really seem quite correct since it is
for things that have a direct native expression to translate into. It looks
like the same effect can be had by instead adding this to
`DruidConvertTable.STANDARD_CONVERTLET_OPERATORS` which just allows it to pass
through to see if anything else handles it (at least all the tests pass),
though i'm not really sure if that is the most appropriate way to do this
either since I guess this does have a window operator associated with it, but
it does seem better than using a `DirectOperatorConversion`.
--
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]
