[GitHub] [incubator-druid] clintropolis opened a new pull request #9043: sql auto limit wrapping fix

[GitBox <gi...@apache.org>]

clintropolis opened a new pull request #9043: sql auto limit wrapping fix
URL: https://github.com/apache/incubator-druid/pull/9043
 
 
   ### Description
   
   This PR fixes a bug I encountered with the SQL auto limit wrapping behavior used by the web console, when pointed at data generated from the ['basic' schema from druid-benchmarks](https://github.com/apache/incubator-druid/blob/master/benchmarks/src/main/java/org/apache/druid/benchmark/datagen/BenchmarkSchemas.java#L45). Out of the box, the query produced by the web console would not plan, but if I either removed _any_ column from the output, or explicitly specified a limit, the query would function correctly. The only difference between setting an explicit limit and using the auto limit was that the `LogicalSort` of the auto limit had an offset of 0 instead of null, and modifying the planner to only set offset on the sort if the offset was non-zero has resolve the issue. This matches the behavior of the sort collapse rule, which only sets offset if it is greater than 0.
   
   However, why removing columns would allow the query to plan prior to the changes in this PR remains a mystery to me and I find it bothersome, but I haven't had a chance to dig into the planner more to find out what is going on.
   
   The added test illustrates this behavior, where the 2nd query does not plan without the changes in this PR. I tried to replicate the issue by just having a lot of columns not in the exact schema as the benchmark generator data, but I was unable to do it by hand with all string dimensions at least, so I ended up copying in 2 rows from the benchmark generator in order to get this unit test in place.
   
   <hr>
   
   This PR has:
   - [x] been self-reviewed.
   - [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
   - [x] added unit tests or modified existing tests to cover new code paths.
   - [x] been tested in a test Druid cluster.
   
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

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