Add broker setting to override default implicit query response limit #14452
Conversation
There was a problem hiding this comment.
A reasonable chunk of the diff is because of rename of:
_queryResponseLimit->_queryResponseLimitOverrideDEFAULT_BROKER_QUERY_RESPONSE_LIMIT->DEFAULT_BROKER_QUERY_RESPONSE_LIMIT_OVERRIDE
The logic is the same though ?
Also the semantics of DEFAULT_BROKER_QUERY_RESPONSE_LIMIT has been changed.
Am I interpreting the diff correctly ?
|
I renamed the variables (but not setting keys) because, in my opinion, existing query response limit is badly named. It is not a default but rather an override because it applies even when sql command contains |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14452 +/- ##
============================================
+ Coverage 61.75% 63.79% +2.04%
- Complexity 207 1565 +1358
============================================
Files 2436 2663 +227
Lines 133233 146219 +12986
Branches 20636 22404 +1768
============================================
+ Hits 82274 93282 +11008
- Misses 44911 46040 +1129
- Partials 6048 6897 +849
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I think I understand the change now. Repeating to verify:
Current Behavior:
If pinot.broker.enable.query.limit.override is set AND PinotQuery.limit > pinot.broker.enable.query.limit, then override the limit.
PR behavior
If pinot.broker.default.query.response.limit is set AND no limit in query, then set limit.
Override logic continues to apply.
A couple of things that threw me off course are:
- Rename to OVERRIDE.
- New config param name has
defaultin it and rest is the same. I assumed it had something to do with default override limit.
|
Current formula is : |
pinot-common/src/main/java/org/apache/pinot/common/request/PinotQuery.java
Outdated
Show resolved
Hide resolved
Got it. And the test cases test the new functionality only. |
| @@ -308,6 +313,10 @@ protected BrokerResponse handleRequest(long requestId, String query, SqlNodeAndO | |||
| } | |||
| } | |||
|
|
|||
| if (isDefaultQueryResponseLimitEnabled() && !pinotQuery.isSetLimit()) { | |||
There was a problem hiding this comment.
Could we instead keep the default value for pinot.broker.default.query.response.limit as 10 and avoid this "enabled" check? Basically we'd then simply set the query limit to whatever the value of the config is (when there's no explicit limit set in the query).
There was a problem hiding this comment.
+1 - I was just about commenting the same thing in the CommonConstants file. That will also make documentation of this feature simpler. No need to explain when this limit is enabled
There was a problem hiding this comment.
That will also make documentation of this feature simpler. No need to explain when this limit is enabled
There's no new config for that here anyway though, the enabled check was simply verifying whether the config value was > -1. The enabled config is for the existing "override" limit config.
There was a problem hiding this comment.
IIUC, the new behaviour will be:
If query does not have a limit clause:
- Limit will be 10.
- If a config parameter with
defaultin its name is set, then the limit will be set to config value
Finally, limit will be overriden by a config param with override in its name.
My suggestion is to remove Limit will be 10 statement
There was a problem hiding this comment.
That's not backward compatible though, the v1 engine has always had a default limit of 10 for queries with no explicit limit set and we can't really safely change that.
Nvm, I think I misunderstood you and you're actually proposing the same thing as me.
| public static final String CONFIG_OF_BROKER_DEFAULT_QUERY_RESPONSE_LIMIT = | ||
| "pinot.broker.default.query.response.limit"; |
There was a problem hiding this comment.
What do you think about naming this pinot.broker.default.query.limit instead? I know that this isn't exactly aligned with the other config name but I agree with you in that it seems poorly named (both the "response" aspect and the fact that it doesn't state that it's an override) and it's fine if we don't stick to the same pattern.
|
|
||
| // this test uses separate cluster because it needs to change broker configuration | ||
| // which is only done once per suite | ||
| public class BrokerQueryLimitTest extends BaseClusterIntegrationTest { |
There was a problem hiding this comment.
Do we really need an entire integration test suite for this? Seems a little heavyweight especially considering Pinot's integration tests have a fairly significant setup time / cost associated with them. We might be okay with simple unit tests for this instead.
There was a problem hiding this comment.
For my information, is there a way to test SSQE queries as unit tests ? I know MSQE queries can be tested using QueryEnvironment
There was a problem hiding this comment.
There was a problem hiding this comment.
This doesn't go through the broker request handler however.
There was a problem hiding this comment.
Actually I just realized that the broker request handler classes don't have a good setup for unit tests and it might be a fair amount of refactoring work to make it properly testable 😞
There was a problem hiding this comment.
Yes, the reason I used integration test is because broker isn't triggered in unit tests .
By default single stage query engine (aka v1) uses implicit limit of 10 rows when no limit is set.
This PR adds
pinot.broker.default.query.response.limitthat allows for overriding it.It only applies when limit is not set explicitly in the query.
By default, the new setting is disabled (via value being lower than 0).