You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/02/01 22:30:38 UTC

[GitHub] [incubator-pinot] harshinielath opened a new pull request #5040: Update Selection Query Limit with pre-configured value

harshinielath opened a new pull request #5040: Update Selection Query Limit with pre-configured value
URL: https://github.com/apache/incubator-pinot/pull/5040
 
 
   Update Selection query limit if MAX_QUERY_SELECTION_LIMIT is set.

----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] codecov-io commented on issue #5040: Update Selection Query Limit with pre-configured value

Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #5040: Update Selection Query Limit with pre-configured value
URL: https://github.com/apache/incubator-pinot/pull/5040#issuecomment-581079322
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=h1) Report
   > Merging [#5040](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/89fec1e375ec480b47508e757974d1cdd65d2c68?src=pr&el=desc) will **decrease** coverage by `19.21%`.
   > The diff coverage is `42.85%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/5040/graphs/tree.svg?width=650&token=4ibza2ugkz&height=150&src=pr)](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=tree)
   
   ```diff
   @@              Coverage Diff              @@
   ##             master    #5040       +/-   ##
   =============================================
   - Coverage     58.69%   39.48%   -19.22%     
   =============================================
     Files          1182     1182               
     Lines         63130    63138        +8     
     Branches       9287     9289        +2     
   =============================================
   - Hits          37055    24930    -12125     
   - Misses        23358    36039    +12681     
   + Partials       2717     2169      -548
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...ava/org/apache/pinot/pql/parsers/Pql2Compiler.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wcWwvcGFyc2Vycy9QcWwyQ29tcGlsZXIuamF2YQ==) | `36% <100%> (-33.13%)` | `0 <0> (ø)` | |
   | [...ache/pinot/pql/parsers/pql2/ast/SelectAstNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wcWwvcGFyc2Vycy9wcWwyL2FzdC9TZWxlY3RBc3ROb2RlLmphdmE=) | `59.18% <33.33%> (-38.72%)` | `0 <0> (ø)` | |
   | [...org/apache/pinot/sql/parsers/CalciteSqlParser.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9DYWxjaXRlU3FsUGFyc2VyLmphdmE=) | `65.84% <33.33%> (-27.47%)` | `0 <0> (ø)` | |
   | [.../org/apache/pinot/common/http/MultiGetRequest.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vaHR0cC9NdWx0aUdldFJlcXVlc3QuamF2YQ==) | `0% <0%> (-100%)` | `0% <0%> (ø)` | |
   | [...che/pinot/common/restlet/resources/TablesList.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVzdGxldC9yZXNvdXJjZXMvVGFibGVzTGlzdC5qYXZh) | `0% <0%> (-100%)` | `0% <0%> (ø)` | |
   | [...che/pinot/pql/parsers/pql2/ast/OptionsAstNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wcWwvcGFyc2Vycy9wcWwyL2FzdC9PcHRpb25zQXN0Tm9kZS5qYXZh) | `0% <0%> (-100%)` | `0% <0%> (ø)` | |
   | [...ot/core/query/scheduler/TableBasedGroupMapper.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9zY2hlZHVsZXIvVGFibGVCYXNlZEdyb3VwTWFwcGVyLmphdmE=) | `0% <0%> (-100%)` | `0% <0%> (ø)` | |
   | [...t/core/segment/index/readers/OnHeapDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L2luZGV4L3JlYWRlcnMvT25IZWFwRGljdGlvbmFyeS5qYXZh) | `0% <0%> (-100%)` | `0% <0%> (ø)` | |
   | [...t/core/query/scheduler/OutOfCapacityException.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9zY2hlZHVsZXIvT3V0T2ZDYXBhY2l0eUV4Y2VwdGlvbi5qYXZh) | `0% <0%> (-100%)` | `0% <0%> (ø)` | |
   | [...ot/core/query/scheduler/SchedulerQueryContext.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9zY2hlZHVsZXIvU2NoZWR1bGVyUXVlcnlDb250ZXh0LmphdmE=) | `0% <0%> (-100%)` | `0% <0%> (ø)` | |
   | ... and [573 more](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=footer). Last update [89fec1e...632df82](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] codecov-io edited a comment on issue #5040: Update Selection Query Limit with pre-configured value

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #5040: Update Selection Query Limit with pre-configured value
URL: https://github.com/apache/incubator-pinot/pull/5040#issuecomment-581079322
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=h1) Report
   > Merging [#5040](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/89fec1e375ec480b47508e757974d1cdd65d2c68?src=pr&el=desc) will **increase** coverage by `0.03%`.
   > The diff coverage is `100%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/5040/graphs/tree.svg?width=650&token=4ibza2ugkz&height=150&src=pr)](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #5040      +/-   ##
   ============================================
   + Coverage     58.69%   58.73%   +0.03%     
     Complexity       12       12              
   ============================================
     Files          1182     1182              
     Lines         63130    63143      +13     
     Branches       9287     9289       +2     
   ============================================
   + Hits          37055    37085      +30     
   + Misses        23358    23342      -16     
   + Partials       2717     2716       -1
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...apache/pinot/common/request/PinotQueryRequest.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVxdWVzdC9QaW5vdFF1ZXJ5UmVxdWVzdC5qYXZh) | `60% <ø> (ø)` | `0 <0> (?)` | |
   | [.../pinot/common/request/PinotQueryParserFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVxdWVzdC9QaW5vdFF1ZXJ5UGFyc2VyRmFjdG9yeS5qYXZh) | `55.55% <100%> (ø)` | `0 <0> (?)` | |
   | [...org/apache/pinot/sql/parsers/CalciteSqlParser.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9DYWxjaXRlU3FsUGFyc2VyLmphdmE=) | `93.41% <100%> (+0.11%)` | `0 <0> (ø)` | :arrow_down: |
   | [...roker/requesthandler/BaseBrokerRequestHandler.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQmFzZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `82.11% <100%> (+0.05%)` | `0 <0> (ø)` | :arrow_down: |
   | [...org/apache/pinot/common/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | `41.17% <100%> (+1.17%)` | `0 <0> (ø)` | :arrow_down: |
   | [...ache/pinot/pql/parsers/pql2/ast/SelectAstNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wcWwvcGFyc2Vycy9wcWwyL2FzdC9TZWxlY3RBc3ROb2RlLmphdmE=) | `97.95% <100%> (+0.06%)` | `0 <0> (ø)` | :arrow_down: |
   | [...ot/controller/helix/core/util/HelixSetupUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3V0aWwvSGVsaXhTZXR1cFV0aWxzLmphdmE=) | `89.02% <100%> (+0.13%)` | `0 <0> (ø)` | :arrow_down: |
   | [.../pinot/broker/broker/helix/HelixBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0hlbGl4QnJva2VyU3RhcnRlci5qYXZh) | `70.39% <100%> (+0.39%)` | `0 <0> (ø)` | :arrow_down: |
   | [...a/manager/realtime/RealtimeSegmentDataManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVTZWdtZW50RGF0YU1hbmFnZXIuamF2YQ==) | `50% <0%> (-25%)` | `0% <0%> (ø)` | |
   | [...impl/dictionary/FloatOffHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRmxvYXRPZmZIZWFwTXV0YWJsZURpY3Rpb25hcnkuamF2YQ==) | `60.21% <0%> (-12.91%)` | `0% <0%> (ø)` | |
   | ... and [21 more](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=footer). Last update [89fec1e...365eede](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] harshinielath commented on issue #5040: Update Selection Query Limit with pre-configured value

Posted by GitBox <gi...@apache.org>.
harshinielath commented on issue #5040: Update Selection Query Limit with pre-configured value
URL: https://github.com/apache/incubator-pinot/pull/5040#issuecomment-583694927
 
 
   > Why do you need to move PinotQueryRequest into pinot-common? And why is that related to this change?
   
   Moved the classes when the limit logic was added in Parsers before. I will move it back. 

----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang commented on issue #5040: Update Selection Query Limit with pre-configured value

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on issue #5040: Update Selection Query Limit with pre-configured value
URL: https://github.com/apache/incubator-pinot/pull/5040#issuecomment-583078559
 
 
   > > We already have the `queryResponseLimit` which serves the same purpose and reject queries with LIMIT or TOP higher than that, do we need another configuration for that?
   > 
   > The purpose of this PR is to protect server side from getting big queries but still return partial results.
   > E.g. once we connect with powerbi&presto, a table scan operator by default will send a select with high limit value.
   
   In that case, we might better introduce some config indicating best-effort querying, and override query with `queryResponseLimit`?

----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] kishoreg commented on issue #5040: Update Selection Query Limit with pre-configured value

Posted by GitBox <gi...@apache.org>.
kishoreg commented on issue #5040: Update Selection Query Limit with pre-configured value
URL: https://github.com/apache/incubator-pinot/pull/5040#issuecomment-583101155
 
 
   What's the advantage of pushing this to clients? The main purpose of this feature is to protect the server against malicious clients right

----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] codecov-io edited a comment on issue #5040: Update Selection Query Limit with pre-configured value

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #5040: Update Selection Query Limit with pre-configured value
URL: https://github.com/apache/incubator-pinot/pull/5040#issuecomment-581079322
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=h1) Report
   > Merging [#5040](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/89fec1e375ec480b47508e757974d1cdd65d2c68?src=pr&el=desc) will **decrease** coverage by `0.13%`.
   > The diff coverage is `77.77%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/5040/graphs/tree.svg?width=650&token=4ibza2ugkz&height=150&src=pr)](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #5040      +/-   ##
   ============================================
   - Coverage     58.69%   58.55%   -0.14%     
     Complexity       12       12              
   ============================================
     Files          1182     1183       +1     
     Lines         63130    63230     +100     
     Branches       9287     9303      +16     
   ============================================
   - Hits          37055    37026      -29     
   - Misses        23358    23488     +130     
   + Partials       2717     2716       -1
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...org/apache/pinot/common/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | `40% <ø> (ø)` | `0 <0> (ø)` | :arrow_down: |
   | [...apache/pinot/common/request/PinotQueryRequest.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVxdWVzdC9QaW5vdFF1ZXJ5UmVxdWVzdC5qYXZh) | `60% <ø> (ø)` | `0 <0> (?)` | |
   | [...org/apache/pinot/sql/parsers/CalciteSqlParser.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9DYWxjaXRlU3FsUGFyc2VyLmphdmE=) | `92.03% <ø> (-1.28%)` | `0 <0> (ø)` | |
   | [...ache/pinot/pql/parsers/pql2/ast/SelectAstNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wcWwvcGFyc2Vycy9wcWwyL2FzdC9TZWxlY3RBc3ROb2RlLmphdmE=) | `97.89% <ø> (ø)` | `0 <0> (ø)` | :arrow_down: |
   | [.../pinot/common/request/PinotQueryParserFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVxdWVzdC9QaW5vdFF1ZXJ5UGFyc2VyRmFjdG9yeS5qYXZh) | `50% <0%> (ø)` | `0 <0> (?)` | |
   | [...pql/parsers/PinotQuery2BrokerRequestConverter.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wcWwvcGFyc2Vycy9QaW5vdFF1ZXJ5MkJyb2tlclJlcXVlc3RDb252ZXJ0ZXIuamF2YQ==) | `88.01% <100%> (+0.11%)` | `0 <0> (ø)` | :arrow_down: |
   | [...ot/controller/helix/core/util/HelixSetupUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3V0aWwvSGVsaXhTZXR1cFV0aWxzLmphdmE=) | `89.02% <100%> (+0.13%)` | `0 <0> (ø)` | :arrow_down: |
   | [.../pinot/broker/broker/helix/HelixBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0hlbGl4QnJva2VyU3RhcnRlci5qYXZh) | `70.39% <100%> (+0.39%)` | `0 <0> (ø)` | :arrow_down: |
   | [...roker/requesthandler/BaseBrokerRequestHandler.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQmFzZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `81.86% <76.92%> (-0.19%)` | `0 <0> (ø)` | |
   | [.../pinot/ingestion/jobs/SegmentPreprocessingJob.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtcGx1Z2lucy9waW5vdC1iYXRjaC1pbmdlc3Rpb24vdjBfZGVwcmVjYXRlZC9waW5vdC1pbmdlc3Rpb24tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9pbmdlc3Rpb24vam9icy9TZWdtZW50UHJlcHJvY2Vzc2luZ0pvYi5qYXZh) | `0% <0%> (-66.67%)` | `0% <0%> (ø)` | |
   | ... and [56 more](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=footer). Last update [89fec1e...9eeeab6](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang commented on issue #5040: Update Selection Query Limit with pre-configured value

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on issue #5040: Update Selection Query Limit with pre-configured value
URL: https://github.com/apache/incubator-pinot/pull/5040#issuecomment-582652983
 
 
   We already have the `queryResponseLimit` which serves the same purpose and reject queries with LIMIT or TOP higher than that, do we need another configuration for that?

----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5040: Update Selection Query Limit with pre-configured value

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5040: Update Selection Query Limit with pre-configured value
URL: https://github.com/apache/incubator-pinot/pull/5040#discussion_r377434076
 
 

 ##########
 File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
 ##########
 @@ -104,6 +103,7 @@
   private final AtomicInteger _numDroppedLog;
 
   private final boolean _enableCaseInsensitivePql;
+  private final boolean _enableBrokerQueryLimitOverride;
 
 Review comment:
   (nit) `_enableQueryLimitOverride`?

----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] kishoreg commented on a change in pull request #5040: Update Selection Query Limit with pre-configured value

Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #5040: Update Selection Query Limit with pre-configured value
URL: https://github.com/apache/incubator-pinot/pull/5040#discussion_r375507771
 
 

 ##########
 File path: pinot-common/src/main/java/org/apache/pinot/pql/parsers/pql2/ast/SelectAstNode.java
 ##########
 @@ -166,6 +167,11 @@ public void updateBrokerRequest(BrokerRequest brokerRequest) {
     } else {
       brokerRequest.setLimit(DEFAULT_RECORD_LIMIT);
     }
+    // Update Selection query limit if MAX_QUERY_SELECTION_LIMIT is set.
+    int limit = brokerRequest.getLimit();
+    if ((PinotQueryParserFactory.MAX_QUERY_SELECTION_LIMIT > 0) && (limit > PinotQueryParserFactory.MAX_QUERY_SELECTION_LIMIT)) {
 
 Review comment:
   this code can be moved to basebrokerrequesthandler

----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] codecov-io edited a comment on issue #5040: Update Selection Query Limit with pre-configured value

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #5040: Update Selection Query Limit with pre-configured value
URL: https://github.com/apache/incubator-pinot/pull/5040#issuecomment-581079322
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=h1) Report
   > Merging [#5040](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/89fec1e375ec480b47508e757974d1cdd65d2c68?src=pr&el=desc) will **decrease** coverage by `19.22%`.
   > The diff coverage is `27.77%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/5040/graphs/tree.svg?width=650&token=4ibza2ugkz&height=150&src=pr)](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=tree)
   
   ```diff
   @@              Coverage Diff              @@
   ##             master    #5040       +/-   ##
   =============================================
   - Coverage     58.69%   39.47%   -19.23%     
   =============================================
     Files          1182     1185        +3     
     Lines         63130    63412      +282     
     Branches       9287     9321       +34     
   =============================================
   - Hits          37055    25031    -12024     
   - Misses        23358    36215    +12857     
   + Partials       2717     2166      -551
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...org/apache/pinot/sql/parsers/CalciteSqlParser.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9DYWxjaXRlU3FsUGFyc2VyLmphdmE=) | `66.13% <ø> (-27.17%)` | `0 <0> (ø)` | |
   | [...org/apache/pinot/common/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | `40% <ø> (ø)` | `0 <0> (ø)` | :arrow_down: |
   | [...broker/requesthandler/PinotQueryParserFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvUGlub3RRdWVyeVBhcnNlckZhY3RvcnkuamF2YQ==) | `50% <0%> (ø)` | `0 <0> (ø)` | :arrow_down: |
   | [...pql/parsers/PinotQuery2BrokerRequestConverter.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wcWwvcGFyc2Vycy9QaW5vdFF1ZXJ5MkJyb2tlclJlcXVlc3RDb252ZXJ0ZXIuamF2YQ==) | `80.18% <100%> (-7.73%)` | `0 <0> (ø)` | |
   | [.../pinot/broker/broker/helix/HelixBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0hlbGl4QnJva2VyU3RhcnRlci5qYXZh) | `67.76% <100%> (-2.24%)` | `0 <0> (ø)` | |
   | [...ot/controller/helix/core/util/HelixSetupUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3V0aWwvSGVsaXhTZXR1cFV0aWxzLmphdmE=) | `89.02% <100%> (+0.13%)` | `0 <0> (ø)` | :arrow_down: |
   | [...roker/requesthandler/BaseBrokerRequestHandler.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQmFzZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `73.65% <7.69%> (-8.41%)` | `0 <0> (ø)` | |
   | [.../org/apache/pinot/common/http/MultiGetRequest.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vaHR0cC9NdWx0aUdldFJlcXVlc3QuamF2YQ==) | `0% <0%> (-100%)` | `0% <0%> (ø)` | |
   | [...che/pinot/common/restlet/resources/TablesList.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVzdGxldC9yZXNvdXJjZXMvVGFibGVzTGlzdC5qYXZh) | `0% <0%> (-100%)` | `0% <0%> (ø)` | |
   | [...che/pinot/pql/parsers/pql2/ast/OptionsAstNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wcWwvcGFyc2Vycy9wcWwyL2FzdC9PcHRpb25zQXN0Tm9kZS5qYXZh) | `0% <0%> (-100%)` | `0% <0%> (ø)` | |
   | ... and [584 more](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=footer). Last update [89fec1e...e906398](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5040: Update Selection Query Limit with pre-configured value

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5040: Update Selection Query Limit with pre-configured value
URL: https://github.com/apache/incubator-pinot/pull/5040#discussion_r377434032
 
 

 ##########
 File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java
 ##########
 @@ -182,7 +181,8 @@ public void start()
         new HelixConfigScopeBuilder(HelixConfigScope.ConfigScopeProperty.CLUSTER).forCluster(_clusterName).build();
     String enableCaseInsensitivePql = configAccessor.get(helixConfigScope, Helix.ENABLE_CASE_INSENSITIVE_PQL_KEY);
     _brokerConf.setProperty(Helix.ENABLE_CASE_INSENSITIVE_PQL_KEY, Boolean.valueOf(enableCaseInsensitivePql));
-
+    String enableBrokerQueryLimitOverride = configAccessor.get(helixConfigScope, Broker.CONFIG_OF_ENABLE_QUERY_LIMIT_OVERRIDE);
 
 Review comment:
   (nit) `enableQueryLimitOverride`

----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] harshinielath commented on issue #5040: Update Selection Query Limit with pre-configured value

Posted by GitBox <gi...@apache.org>.
harshinielath commented on issue #5040: Update Selection Query Limit with pre-configured value
URL: https://github.com/apache/incubator-pinot/pull/5040#issuecomment-584696130
 
 
   > LGTM otherwise, thanks
   
   Done.. Thank you!

----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] kishoreg commented on issue #5040: Update Selection Query Limit with pre-configured value

Posted by GitBox <gi...@apache.org>.
kishoreg commented on issue #5040: Update Selection Query Limit with pre-configured value
URL: https://github.com/apache/incubator-pinot/pull/5040#issuecomment-581090370
 
 
   Good feature.  Instead of having to set this as part of the JVM start-up parameter, it's probably better to have this configuration at the cluster level in Helix. 

----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] kishoreg commented on a change in pull request #5040: Update Selection Query Limit with pre-configured value

Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #5040: Update Selection Query Limit with pre-configured value
URL: https://github.com/apache/incubator-pinot/pull/5040#discussion_r375507354
 
 

 ##########
 File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
 ##########
 @@ -122,6 +123,9 @@ public BaseBrokerRequestHandler(Configuration config, RoutingTable routingTable,
     } else {
       _tableCache = null;
     }
+
+    PinotQueryParserFactory.MAX_QUERY_SELECTION_LIMIT = _config.getInt(CommonConstants.Helix.MAX_QUERY_SELECTION_LIMIT_KEY, CommonConstants.Helix.DEFAULT_MAX_QUERY_SELECTION_LIMIT);
 
 Review comment:
   we dont need to set the variable in PinotQueryParserFactory. Just change the brokerrequest using the config

----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5040: Update Selection Query Limit with pre-configured value

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5040: Update Selection Query Limit with pre-configured value
URL: https://github.com/apache/incubator-pinot/pull/5040#discussion_r377433700
 
 

 ##########
 File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
 ##########
 @@ -122,6 +122,9 @@ public BaseBrokerRequestHandler(Configuration config, RoutingTable routingTable,
     } else {
       _tableCache = null;
     }
+
+    _enableBrokerQueryLimitOverride = _config.getBoolean(CommonConstants.Broker.CONFIG_OF_ENABLE_QUERY_LIMIT_OVERRIDE, false);
 
 Review comment:
   (nit) `CommonConstants.Broker` is already imported so you can directly use `Broker.CONFIG_OF_ENABLE_QUERY_LIMIT_OVERRIDE`

----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5040: Update Selection Query Limit with pre-configured value

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5040: Update Selection Query Limit with pre-configured value
URL: https://github.com/apache/incubator-pinot/pull/5040#discussion_r376888445
 
 

 ##########
 File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
 ##########
 @@ -42,7 +42,7 @@
 
     public static final String LEAD_CONTROLLER_RESOURCE_ENABLED_KEY = "RESOURCE_ENABLED";
     public static final String ENABLE_CASE_INSENSITIVE_PQL_KEY = "enable.case.insensitive.pql";
-
+    public static final String ENABLE_BROKER_QUERY_LIMIT_OVERRIDE_KEY = "enable.broker.query.limit.override";
 
 Review comment:
   Can you please move this under class `Broker` instead of `Helix`? Also, I would recommend changing it to `CONFIG_OF_ENABLE_QUERY_LIMIT_OVERRIDE` to be consistent with other configs.

----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5040: Update Selection Query Limit with pre-configured value

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5040: Update Selection Query Limit with pre-configured value
URL: https://github.com/apache/incubator-pinot/pull/5040#discussion_r377433522
 
 

 ##########
 File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
 ##########
 @@ -166,6 +165,7 @@ public ServerType getServerType() {
     public static final String CONFIG_OF_BROKER_MIN_RESOURCE_PERCENT_FOR_START =
         "pinot.broker.startup.minResourcePercent";
     public static final double DEFAULT_BROKER_MIN_RESOURCE_PERCENT_FOR_START = 100.0;
+    public static final String CONFIG_OF_ENABLE_QUERY_LIMIT_OVERRIDE = "enable.broker.query.limit.override";
 
 Review comment:
   `pinot.broker.enable.query.limit.override` for consistency

----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 commented on issue #5040: Update Selection Query Limit with pre-configured value

Posted by GitBox <gi...@apache.org>.
fx19880617 commented on issue #5040: Update Selection Query Limit with pre-configured value
URL: https://github.com/apache/incubator-pinot/pull/5040#issuecomment-582687159
 
 
   > We already have the `queryResponseLimit` which serves the same purpose and reject queries with LIMIT or TOP higher than that, do we need another configuration for that?
   
   The purpose of this PR is to protect server side from getting big queries but still return partial results.
   E.g. once we connect with powerbi&presto, a table scan operator by default will send a select with high limit value.
   

----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 merged pull request #5040: Update Selection Query Limit with pre-configured value

Posted by GitBox <gi...@apache.org>.
fx19880617 merged pull request #5040: Update Selection Query Limit with pre-configured value
URL: https://github.com/apache/incubator-pinot/pull/5040
 
 
   

----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] codecov-io edited a comment on issue #5040: Update Selection Query Limit with pre-configured value

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #5040: Update Selection Query Limit with pre-configured value
URL: https://github.com/apache/incubator-pinot/pull/5040#issuecomment-581079322
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=h1) Report
   > Merging [#5040](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/89fec1e375ec480b47508e757974d1cdd65d2c68?src=pr&el=desc) will **decrease** coverage by `0.18%`.
   > The diff coverage is `77.77%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/5040/graphs/tree.svg?width=650&token=4ibza2ugkz&height=150&src=pr)](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #5040      +/-   ##
   ============================================
   - Coverage     58.69%   58.51%   -0.19%     
     Complexity       12       12              
   ============================================
     Files          1182     1183       +1     
     Lines         63130    63230     +100     
     Branches       9287     9303      +16     
   ============================================
   - Hits          37055    36998      -57     
   - Misses        23358    23513     +155     
   - Partials       2717     2719       +2
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...pinot/broker/requesthandler/PinotQueryRequest.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvUGlub3RRdWVyeVJlcXVlc3QuamF2YQ==) | `60% <ø> (ø)` | `0 <0> (?)` | |
   | [...org/apache/pinot/common/utils/CommonConstants.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | `40% <ø> (ø)` | `0 <0> (ø)` | :arrow_down: |
   | [...org/apache/pinot/sql/parsers/CalciteSqlParser.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9DYWxjaXRlU3FsUGFyc2VyLmphdmE=) | `92.03% <ø> (-1.28%)` | `0 <0> (ø)` | |
   | [...broker/requesthandler/PinotQueryParserFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvUGlub3RRdWVyeVBhcnNlckZhY3RvcnkuamF2YQ==) | `50% <0%> (ø)` | `0 <0> (?)` | |
   | [...pql/parsers/PinotQuery2BrokerRequestConverter.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wcWwvcGFyc2Vycy9QaW5vdFF1ZXJ5MkJyb2tlclJlcXVlc3RDb252ZXJ0ZXIuamF2YQ==) | `88.01% <100%> (+0.11%)` | `0 <0> (ø)` | :arrow_down: |
   | [.../pinot/broker/broker/helix/HelixBrokerStarter.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0hlbGl4QnJva2VyU3RhcnRlci5qYXZh) | `70.39% <100%> (+0.39%)` | `0 <0> (ø)` | :arrow_down: |
   | [...ot/controller/helix/core/util/HelixSetupUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3V0aWwvSGVsaXhTZXR1cFV0aWxzLmphdmE=) | `89.02% <100%> (+0.13%)` | `0 <0> (ø)` | :arrow_down: |
   | [...roker/requesthandler/BaseBrokerRequestHandler.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQmFzZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `81.86% <76.92%> (-0.19%)` | `0 <0> (ø)` | |
   | [.../pinot/ingestion/jobs/SegmentPreprocessingJob.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtcGx1Z2lucy9waW5vdC1iYXRjaC1pbmdlc3Rpb24vdjBfZGVwcmVjYXRlZC9waW5vdC1pbmdlc3Rpb24tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9pbmdlc3Rpb24vam9icy9TZWdtZW50UHJlcHJvY2Vzc2luZ0pvYi5qYXZh) | `0% <0%> (-66.67%)` | `0% <0%> (ø)` | |
   | [...he/pinot/ingestion/utils/JobPreparationHelper.java](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree#diff-cGlub3QtcGx1Z2lucy9waW5vdC1iYXRjaC1pbmdlc3Rpb24vdjBfZGVwcmVjYXRlZC9waW5vdC1pbmdlc3Rpb24tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9pbmdlc3Rpb24vdXRpbHMvSm9iUHJlcGFyYXRpb25IZWxwZXIuamF2YQ==) | `0% <0%> (-64.29%)` | `0% <0%> (ø)` | |
   | ... and [54 more](https://codecov.io/gh/apache/incubator-pinot/pull/5040/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=footer). Last update [89fec1e...1c6bc6c](https://codecov.io/gh/apache/incubator-pinot/pull/5040?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 commented on issue #5040: Update Selection Query Limit with pre-configured value

Posted by GitBox <gi...@apache.org>.
fx19880617 commented on issue #5040: Update Selection Query Limit with pre-configured value
URL: https://github.com/apache/incubator-pinot/pull/5040#issuecomment-583095773
 
 
   > > > We already have the `queryResponseLimit` which serves the same purpose and reject queries with LIMIT or TOP higher than that, do we need another configuration for that?
   > > 
   > > 
   > > The purpose of this PR is to protect server side from getting big queries but still return partial results.
   > > E.g. once we connect with powerbi&presto, a table scan operator by default will send a select with high limit value.
   > 
   > In that case, we might better introduce some config indicating best-effort querying, and override query with `queryResponseLimit`?
   
   Make sense, we can change the parameter to enable it.

----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org