fix bug with sqlOuterLimit, use sqlOuterLimit in web console#8919
fix bug with sqlOuterLimit, use sqlOuterLimit in web console#8919gianm merged 7 commits intoapache:masterfrom
Conversation
… query for web console
|
👍 on the console changes! |
|
|
||
| if (!isEmptyContext(queryContext)) { | ||
| if (!isEmptyContext(queryContext) || wrapQueryLimit) { | ||
| jsonQuery.context = Object.assign(jsonQuery.context || {}, queryContext); |
There was a problem hiding this comment.
May be better to do:
jsonQuery.context = Object.assign({}, jsonQuery.context || {}, queryContext);So there is no chance of changing something silly in pleace
|
|
||
| if (!isEmptyContext(queryContext)) explainPayload.context = queryContext; | ||
| if (!isEmptyContext(queryContext) || wrapQueryLimit) { | ||
| explainPayload.context = queryContext || {}; |
There was a problem hiding this comment.
| explainPayload.context = queryContext || {}; | |
| explainPayload.context = Object.assign({}, queryContext || {}); |
Otherwise you are changing the queryContext state variable in place if it is set. Maybe not but still cleaner to avoid even the possibility of that.
| new Object[]{"abc", 1L} | ||
| ); | ||
| } | ||
| testQuery( |
There was a problem hiding this comment.
I think it's best to have one testQuery call per test method. So, I'd create a new method for this test.
There was a problem hiding this comment.
split the extra one already there, the one i added into separate tests, also added a few more
| return LogicalSort.create( | ||
| outerSort.getInput(), | ||
| outerSort.collation, | ||
| makeBigIntLiteral(0), |
There was a problem hiding this comment.
This won't handle offsets properly, nor will it handle properly the case where the outer limit is higher than the inner limit. Check out SortCollapseRule for some logic that handles both of these cases. You might want to create a helper method and use it in both places.
There was a problem hiding this comment.
pushed some repeated code to extract fetch and offset and fetch combining logic from SortCollapseRule to Calcites
|
TS side looks great 👍 thank you for addressing my feedback |
gianm
left a comment
gianm
left a comment
There was a problem hiding this comment.
LGTM, thanks @clintropolis
3 participants
Description
This PR modifies the 'smart limit' strategy in the web-console query view to use the
sqlOuterLimitparameter introduced in #8566. It also fixes a bug withsqlOuterLimitif the outer rel is already aLogicalSortwhich would cause a query which should plan to a topN but without a limit set to be unplannable. The logic now will modify the limit of an outer sort instead of wrapping it in another sort.This PR has: