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 2021/03/29 03:56:10 UTC

[GitHub] [incubator-pinot] mayankshriv opened a new pull request #6721: WIP: Pinot SPI refactor.

mayankshriv opened a new pull request #6721:
URL: https://github.com/apache/incubator-pinot/pull/6721


   In the first phase of Pinot SPI refactor, we introduced the plugin based architecture. This is
   a continuation of the refactor. Details of the refactor can be found here:
   https://docs.google.com/document/d/1urROfQZuTE8JJmW3IMCeB2i3FYoEyG1TCyPsxvSaNuw/edit?usp=sharing
   
   In this second phase, we will mainly be focuing on the following:
   - We will create pinot-segment-spi and pinot-query-spi modules that will host all the segment
     and query related interfaces.
   - We will also create pinot-segment-default and pinot-query-default which will host the default (current)
     implementations of Pinot segment and query execution engine.
   - These will purely be refactors and would not create any function differences. However, to clean up
     dependency issues in existing code, we may make some changes (eg create a pojo), but those also
     should not modify any functionality.
   
   In this particular PR, we are making the folowing changes:
   - Introduced pinot-segment-spi.
   - Moved all segment related interaces from pinot-core to pinot-segment-spi.
   - In order to clean dependency issues, we have also introduced pinot-query-api, but in this PR
     we are only moving bare minimal files needed for the pinot-segment-spi move.
   
   ## Description
   Add a description of your PR here.
   A good description should include pointers to an issue or design document, etc.
   ## Upgrade Notes
   Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
   * [ ] Yes (Please label as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
   
   Does this PR fix a zero-downtime upgrade introduced earlier?
   * [ ] Yes (Please label this as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
   
   Does this PR otherwise need attention when creating release notes? Things to consider:
   - New configuration options
   - Deprecation of configurations
   - Signature changes to public methods/interfaces
   - New plugins added or old plugins removed
   * [ ] Yes (Please label this PR as **<code>release-notes</code>** and complete the section on Release Notes)
   ## Release Notes
   If you have tagged this as either backward-incompat or release-notes,
   you MUST add text here that you would like to see appear in release notes of the
   next release.
   
   If you have a series of commits adding or enabling a feature, then
   add this section only in final commit that marks the feature completed.
   Refer to earlier release notes to see examples of text
   
   ## Documentation
   If you have introduced a new feature or configuration, please add it to the documentation as well.
   See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document
   


-- 
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



---------------------------------------------------------------------
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 #6721: WIP: Pinot SPI refactor (no functionality change)

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6721:
URL: https://github.com/apache/incubator-pinot/pull/6721#discussion_r606360983



##########
File path: pinot-common/src/test/java/org/apache/pinot/common/function/AggregationFunctionTypeTest.java
##########
@@ -18,10 +18,9 @@
  */
 package org.apache.pinot.common.function;
 
+import org.apache.pinot.segment.spi.AggregationFunctionType;

Review comment:
       Let's a move the tests?

##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/recommender/rules/impl/BloomFilterRule.java
##########
@@ -24,11 +24,11 @@
 import org.apache.pinot.controller.recommender.rules.AbstractRule;
 import org.apache.pinot.controller.recommender.rules.io.params.BloomFilterRuleParams;
 import org.apache.pinot.controller.recommender.rules.utils.FixedLenBitset;
-import org.apache.pinot.core.query.request.context.ExpressionContext;
-import org.apache.pinot.core.query.request.context.FilterContext;
 import org.apache.pinot.core.query.request.context.QueryContext;
-import org.apache.pinot.core.query.request.context.predicate.Predicate;
 import org.apache.pinot.core.requesthandler.BrokerRequestOptimizer;
+import org.apache.pinot.segment.spi.misc.ExpressionContext;
+import org.apache.pinot.query.spi.FilterContext;
+import org.apache.pinot.segment.spi.misc.Predicate;

Review comment:
       Why are we moving these 3 classes into different packages? They should be under the query-spi if you need to move them




-- 
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



---------------------------------------------------------------------
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 #6721: WIP: Pinot SPI refactor.

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6721:
URL: https://github.com/apache/incubator-pinot/pull/6721#discussion_r603474065



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/pruner/PartitionZKMetadataPruner.java
##########
@@ -26,7 +26,7 @@
 import org.apache.pinot.common.metadata.segment.SegmentZKMetadata;
 import org.apache.pinot.common.request.FilterOperator;
 import org.apache.pinot.common.utils.request.FilterQueryTree;
-import org.apache.pinot.core.data.partition.PartitionFunction;
+import org.apache.pinot.query.spi.PartitionFunction;

Review comment:
       We should re-order the imports for all the changed files. You can do it in IDE in one-step by selecting all the changed local files under `Git` panel and use `optimize imports`




-- 
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



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


[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #6721: WIP: Pinot SPI refactor (no functionality change)

Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #6721:
URL: https://github.com/apache/incubator-pinot/pull/6721#discussion_r606371890



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/recommender/rules/impl/BloomFilterRule.java
##########
@@ -24,11 +24,11 @@
 import org.apache.pinot.controller.recommender.rules.AbstractRule;
 import org.apache.pinot.controller.recommender.rules.io.params.BloomFilterRuleParams;
 import org.apache.pinot.controller.recommender.rules.utils.FixedLenBitset;
-import org.apache.pinot.core.query.request.context.ExpressionContext;
-import org.apache.pinot.core.query.request.context.FilterContext;
 import org.apache.pinot.core.query.request.context.QueryContext;
-import org.apache.pinot.core.query.request.context.predicate.Predicate;
 import org.apache.pinot.core.requesthandler.BrokerRequestOptimizer;
+import org.apache.pinot.segment.spi.misc.ExpressionContext;
+import org.apache.pinot.query.spi.FilterContext;
+import org.apache.pinot.segment.spi.misc.Predicate;

Review comment:
       Sorry, based on comments I was removing the `pinot-query-spi` module I had added to park these files. Somehow, IDE did not fix the move correctly. It is fixed now.




-- 
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



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


[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #6721: WIP: Pinot SPI refactor (no functionality change)

Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #6721:
URL: https://github.com/apache/incubator-pinot/pull/6721#discussion_r606390416



##########
File path: pinot-common/src/test/java/org/apache/pinot/common/function/AggregationFunctionTypeTest.java
##########
@@ -18,10 +18,9 @@
  */
 package org.apache.pinot.common.function;
 
+import org.apache.pinot.segment.spi.AggregationFunctionType;

Review comment:
       Done.




-- 
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



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


[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #6721: WIP: Pinot SPI refactor (no functionality change)

Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #6721:
URL: https://github.com/apache/incubator-pinot/pull/6721#discussion_r606372311



##########
File path: pinot-common/src/test/java/org/apache/pinot/common/function/AggregationFunctionTypeTest.java
##########
@@ -18,10 +18,9 @@
  */
 package org.apache.pinot.common.function;
 
+import org.apache.pinot.segment.spi.AggregationFunctionType;

Review comment:
       Ah yes, even though the intention was to only move the interfaces, some POJOs got moved as well. I'll move their tests to the new move.




-- 
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



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


[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #6721: WIP: Pinot SPI refactor (no functionality change)

Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #6721:
URL: https://github.com/apache/incubator-pinot/pull/6721#discussion_r603627158



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/pruner/PartitionZKMetadataPruner.java
##########
@@ -26,7 +26,7 @@
 import org.apache.pinot.common.metadata.segment.SegmentZKMetadata;
 import org.apache.pinot.common.request.FilterOperator;
 import org.apache.pinot.common.utils.request.FilterQueryTree;
-import org.apache.pinot.core.data.partition.PartitionFunction;
+import org.apache.pinot.query.spi.PartitionFunction;

Review comment:
       I'd rather leave formatting out of this huge refactor @mqliang. We should definitely set it up outside the scope of this PR though.




-- 
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



---------------------------------------------------------------------
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 #6721: WIP: Pinot SPI refactor (no functionality change)

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6721:
URL: https://github.com/apache/incubator-pinot/pull/6721#discussion_r606482790



##########
File path: pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/StatsCollectorConfig.java
##########
@@ -87,4 +82,7 @@ public Schema getSchema() {
   public TableConfig getTableConfig() {
     return _tableConfig;
   }
+
+  public void getNumPartitions() {

Review comment:
       Remove this function




-- 
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



---------------------------------------------------------------------
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 #6721: WIP: Pinot SPI refactor (no functionality change)

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6721:
URL: https://github.com/apache/incubator-pinot/pull/6721#discussion_r606482994



##########
File path: pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/StatsCollectorConfig.java
##########
@@ -58,14 +55,12 @@ public FieldSpec getFieldSpecForColumn(String column) {
   }
 
   @Nullable
-  public PartitionFunction getPartitionFunction(String column) {
+  public String getPartitionFunction(String column) {

Review comment:
       Rename it to `getPartitionFunctionName`

##########
File path: pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/StatsCollectorConfig.java
##########
@@ -87,4 +82,7 @@ public Schema getSchema() {
   public TableConfig getTableConfig() {
     return _tableConfig;
   }
+
+  public void getNumPartitions() {

Review comment:
       Remove this function




-- 
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



---------------------------------------------------------------------
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 pull request #6721: WIP: Pinot SPI refactor (no functionality change)

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #6721:
URL: https://github.com/apache/incubator-pinot/pull/6721#issuecomment-812751014


   Next step (next PR), we should move `ColumnMetadata` into `segment-spi` and move some functions in `SegmentMetadataImpl` into the `SegmentMetadata` interface. Currently we have to cast the `SegmentMetadata` to `SegmentMetadataImpl` in order to use those functions.


-- 
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



---------------------------------------------------------------------
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 pull request #6721: WIP: Pinot SPI refactor (no functionality change)

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6721:
URL: https://github.com/apache/incubator-pinot/pull/6721#issuecomment-812202487


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6721?src=pr&el=h1) Report
   > Merging [#6721](https://codecov.io/gh/apache/incubator-pinot/pull/6721?src=pr&el=desc) (872e731) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/71463492749607314b4ae171b0c02e7bbac74335?el=desc) (7146349) will **decrease** coverage by `7.81%`.
   > The diff coverage is `0.00%`.
   
   > :exclamation: Current head 872e731 differs from pull request most recent head 75c1f70. Consider uploading reports for the commit 75c1f70 to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/6721/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/6721?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #6721      +/-   ##
   ============================================
   - Coverage     73.80%   65.98%   -7.82%     
     Complexity       12       12              
   ============================================
     Files          1402     1404       +2     
     Lines         68082    68142      +60     
     Branches       9836     9840       +4     
   ============================================
   - Hits          50245    44961    -5284     
   - Misses        14576    19998    +5422     
   + Partials       3261     3183      -78     
   ```
   
   | Flag | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | integration | `?` | `?` | |
   | unittests | `65.98% <0.00%> (-0.05%)` | `0.00 <0.00> (ø)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6721?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...pinot/broker/pruner/PartitionZKMetadataPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6721/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcHJ1bmVyL1BhcnRpdGlvblpLTWV0YWRhdGFQcnVuZXIuamF2YQ==) | `33.33% <ø> (ø)` | `0.00 <0.00> (ø)` | |
   | [...roker/requesthandler/BaseBrokerRequestHandler.java](https://codecov.io/gh/apache/incubator-pinot/pull/6721/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQmFzZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `19.68% <ø> (-55.72%)` | `0.00 <0.00> (ø)` | |
   | [.../routing/segmentpruner/PartitionSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6721/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1BhcnRpdGlvblNlZ21lbnRQcnVuZXIuamF2YQ==) | `62.06% <ø> (-10.35%)` | `0.00 <0.00> (ø)` | |
   | [...r/routing/segmentpruner/interval/IntervalTree.java](https://codecov.io/gh/apache/incubator-pinot/pull/6721/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsVHJlZS5qYXZh) | `88.88% <ø> (ø)` | `0.00 <0.00> (ø)` | |
   | [...ot/common/function/FunctionDefinitionRegistry.java](https://codecov.io/gh/apache/incubator-pinot/pull/6721/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRnVuY3Rpb25EZWZpbml0aW9uUmVnaXN0cnkuamF2YQ==) | `88.88% <ø> (ø)` | `0.00 <0.00> (ø)` | |
   | [...pql/parsers/PinotQuery2BrokerRequestConverter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6721/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wcWwvcGFyc2Vycy9QaW5vdFF1ZXJ5MkJyb2tlclJlcXVlc3RDb252ZXJ0ZXIuamF2YQ==) | `92.30% <ø> (ø)` | `0.00 <0.00> (ø)` | |
   | [...inot/pql/parsers/pql2/ast/FunctionCallAstNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6721/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wcWwvcGFyc2Vycy9wcWwyL2FzdC9GdW5jdGlvbkNhbGxBc3ROb2RlLmphdmE=) | `59.18% <ø> (ø)` | `0.00 <0.00> (ø)` | |
   | [...org/apache/pinot/sql/parsers/CalciteSqlParser.java](https://codecov.io/gh/apache/incubator-pinot/pull/6721/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9DYWxjaXRlU3FsUGFyc2VyLmphdmE=) | `92.00% <ø> (ø)` | `0.00 <0.00> (ø)` | |
   | [...ces/PinotSegmentUploadDownloadRestletResource.java](https://codecov.io/gh/apache/incubator-pinot/pull/6721/diff?src=pr&el=tree#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90U2VnbWVudFVwbG9hZERvd25sb2FkUmVzdGxldFJlc291cmNlLmphdmE=) | `10.50% <ø> (-44.50%)` | `0.00 <0.00> (ø)` | |
   | [.../pinot/controller/api/upload/SegmentValidator.java](https://codecov.io/gh/apache/incubator-pinot/pull/6721/diff?src=pr&el=tree#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvdXBsb2FkL1NlZ21lbnRWYWxpZGF0b3IuamF2YQ==) | `0.00% <ø> (-62.50%)` | `0.00 <0.00> (ø)` | |
   | ... and [704 more](https://codecov.io/gh/apache/incubator-pinot/pull/6721/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6721?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/6721?src=pr&el=footer). Last update [7146349...75c1f70](https://codecov.io/gh/apache/incubator-pinot/pull/6721?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



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


[GitHub] [incubator-pinot] mqliang commented on a change in pull request #6721: WIP: Pinot SPI refactor.

Posted by GitBox <gi...@apache.org>.
mqliang commented on a change in pull request #6721:
URL: https://github.com/apache/incubator-pinot/pull/6721#discussion_r603481787



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/pruner/PartitionZKMetadataPruner.java
##########
@@ -26,7 +26,7 @@
 import org.apache.pinot.common.metadata.segment.SegmentZKMetadata;
 import org.apache.pinot.common.request.FilterOperator;
 import org.apache.pinot.common.utils.request.FilterQueryTree;
-import org.apache.pinot.core.data.partition.PartitionFunction;
+import org.apache.pinot.query.spi.PartitionFunction;

Review comment:
       There are lots of files are not formatted properly, engineer often forget to format it before commit. How about let's setup a Github workflow, or a maven plugin to ensure all files get formatted:
   
   CLI tools: https://github.com/google/google-java-format
   Maven plugins:
   https://github.com/diffplug/spotless/tree/main/plugin-maven
   https://github.com/coveooss/fmt-maven-plugin
   https://github.com/talios/googleformatter-maven-plugin
   https://github.com/Cosium/git-code-format-maven-plugin




-- 
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



---------------------------------------------------------------------
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 #6721: WIP: Pinot SPI refactor (no functionality change)

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6721:
URL: https://github.com/apache/incubator-pinot/pull/6721#discussion_r604481375



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/pruner/PartitionZKMetadataPruner.java
##########
@@ -26,7 +26,7 @@
 import org.apache.pinot.common.metadata.segment.SegmentZKMetadata;
 import org.apache.pinot.common.request.FilterOperator;
 import org.apache.pinot.common.utils.request.FilterQueryTree;
-import org.apache.pinot.core.data.partition.PartitionFunction;
+import org.apache.pinot.query.spi.PartitionFunction;

Review comment:
       Oh, didn't notice you already created an issue for it. Thanks!




-- 
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



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


[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #6721: WIP: Pinot SPI refactor (no functionality change)

Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #6721:
URL: https://github.com/apache/incubator-pinot/pull/6721#discussion_r605190270



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/pruner/PartitionZKMetadataPruner.java
##########
@@ -26,7 +26,7 @@
 import org.apache.pinot.common.metadata.segment.SegmentZKMetadata;
 import org.apache.pinot.common.request.FilterOperator;
 import org.apache.pinot.common.utils.request.FilterQueryTree;
-import org.apache.pinot.core.data.partition.PartitionFunction;
+import org.apache.pinot.query.spi.PartitionFunction;

Review comment:
       @Jackie-Jiang Ok, I tried to do that, and it did not create unnecessary diffs (as imports were already optimized). Updated the PR after rebasing and optimizing imports.




-- 
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



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


[GitHub] [incubator-pinot] mqliang commented on a change in pull request #6721: WIP: Pinot SPI refactor.

Posted by GitBox <gi...@apache.org>.
mqliang commented on a change in pull request #6721:
URL: https://github.com/apache/incubator-pinot/pull/6721#discussion_r603481787



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/pruner/PartitionZKMetadataPruner.java
##########
@@ -26,7 +26,7 @@
 import org.apache.pinot.common.metadata.segment.SegmentZKMetadata;
 import org.apache.pinot.common.request.FilterOperator;
 import org.apache.pinot.common.utils.request.FilterQueryTree;
-import org.apache.pinot.core.data.partition.PartitionFunction;
+import org.apache.pinot.query.spi.PartitionFunction;

Review comment:
       There are lots of files are not formatted properly, engineer often forget to format it before commit. How about let's setup a Github workflow, or a maven plugin to ensure all file get formatted:
   
   CLI tools: https://github.com/google/google-java-format
   Maven plugins:
   https://github.com/diffplug/spotless/tree/main/plugin-maven
   https://github.com/coveooss/fmt-maven-plugin
   https://github.com/talios/googleformatter-maven-plugin
   https://github.com/Cosium/git-code-format-maven-plugin




-- 
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



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


[GitHub] [incubator-pinot] mqliang commented on a change in pull request #6721: WIP: Pinot SPI refactor (no functionality change)

Posted by GitBox <gi...@apache.org>.
mqliang commented on a change in pull request #6721:
URL: https://github.com/apache/incubator-pinot/pull/6721#discussion_r603630593



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/pruner/PartitionZKMetadataPruner.java
##########
@@ -26,7 +26,7 @@
 import org.apache.pinot.common.metadata.segment.SegmentZKMetadata;
 import org.apache.pinot.common.request.FilterOperator;
 import org.apache.pinot.common.utils.request.FilterQueryTree;
-import org.apache.pinot.core.data.partition.PartitionFunction;
+import org.apache.pinot.query.spi.PartitionFunction;

Review comment:
       Sure. Reformatting all files will do a huge disruption to the code base. Let's do it separately, create a issue to track it: https://github.com/apache/incubator-pinot/issues/6727




-- 
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



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


[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #6721: WIP: Pinot SPI refactor (no functionality change)

Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #6721:
URL: https://github.com/apache/incubator-pinot/pull/6721#discussion_r603623861



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/pruner/PartitionZKMetadataPruner.java
##########
@@ -26,7 +26,7 @@
 import org.apache.pinot.common.metadata.segment.SegmentZKMetadata;
 import org.apache.pinot.common.request.FilterOperator;
 import org.apache.pinot.common.utils.request.FilterQueryTree;
-import org.apache.pinot.core.data.partition.PartitionFunction;
+import org.apache.pinot.query.spi.PartitionFunction;

Review comment:
       @Jackie-Jiang There are specific requests to not make any `OCD` changes that are not related to refactor. I am fine to make that change if optimizing imports is allowed cc: @mcvsubbu 




-- 
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



---------------------------------------------------------------------
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 #6721: WIP: Pinot SPI refactor (no functionality change)

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6721:
URL: https://github.com/apache/incubator-pinot/pull/6721#discussion_r604475827



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/pruner/PartitionZKMetadataPruner.java
##########
@@ -26,7 +26,7 @@
 import org.apache.pinot.common.metadata.segment.SegmentZKMetadata;
 import org.apache.pinot.common.request.FilterOperator;
 import org.apache.pinot.common.utils.request.FilterQueryTree;
-import org.apache.pinot.core.data.partition.PartitionFunction;
+import org.apache.pinot.query.spi.PartitionFunction;

Review comment:
       @mayankshriv All these imports are **related** to this refactor as the imports order are correct before this refactor, but because of the package name change, they become incorrect.
   @mqliang Great suggestion on forcing the format using maven plugin. Can you please open an issue for that (a PR is even better :-P)?




-- 
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



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


[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #6721: WIP: Pinot SPI refactor (no functionality change)

Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #6721:
URL: https://github.com/apache/incubator-pinot/pull/6721#discussion_r605190270



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/pruner/PartitionZKMetadataPruner.java
##########
@@ -26,7 +26,7 @@
 import org.apache.pinot.common.metadata.segment.SegmentZKMetadata;
 import org.apache.pinot.common.request.FilterOperator;
 import org.apache.pinot.common.utils.request.FilterQueryTree;
-import org.apache.pinot.core.data.partition.PartitionFunction;
+import org.apache.pinot.query.spi.PartitionFunction;

Review comment:
       @Jackie-Jiang Ok, I'll do that before merging, so that it is easier to review and does not flag reordering based changes.




-- 
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



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


[GitHub] [incubator-pinot] mayankshriv commented on pull request #6721: WIP: Pinot SPI refactor (no functionality change)

Posted by GitBox <gi...@apache.org>.
mayankshriv commented on pull request #6721:
URL: https://github.com/apache/incubator-pinot/pull/6721#issuecomment-812656301


   > @mayankshriv since it is impossible to go through the changed files, can you please list any non-IDE/manual changes you made? Otherwise, I assume all the other changes were moving files from one package to another and adding pom files (has to be non-ide if we introduced a new module)
   
   @mcvsubbu Explicitly listed the manual changes. All other changes are IDE refactoring.


-- 
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



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


[GitHub] [incubator-pinot] mcvsubbu commented on pull request #6721: WIP: Pinot SPI refactor (no functionality change)

Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #6721:
URL: https://github.com/apache/incubator-pinot/pull/6721#issuecomment-812594882


   @mayankshriv since it is impossible to go through the changed files, can you please list any non-IDE/manual  changes you made? Otherwise, I assume all the other changes were moving files from one package to another and adding pom files (has to be non-ide if we introduced a new module)


-- 
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



---------------------------------------------------------------------
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 #6721: WIP: Pinot SPI refactor (no functionality change)

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6721:
URL: https://github.com/apache/incubator-pinot/pull/6721#discussion_r606484228



##########
File path: docs/customizations.rst
##########
@@ -73,7 +73,7 @@ You can also write your own by implementing the ``org.apache.pinot.controller.he
 
 6. Data partitioning strategies
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-We have various algorithms to partition data during segment creation, as listed under the interface `PartitionFunction <https://github.com/apache/incubator-pinot/blob/master/pinot-core/src/main/java/org/apache/pinot/core/data/partition/PartitionFunction.java>`_. No partitioning is done by default. You can write your own partitioning function by implementing the interface for ``org.apache.pinot.core.data.partition.PartitionFunction``. The partitioning function can be configured on the required columns in the table config as
+We have various algorithms to partition data during segment creation, as listed under the interface `PartitionFunction <https://github.com/apache/incubator-pinot/blob/master/pinot-core/src/main/java/org/apache/pinot/core/data/partition/PartitionFunction.java>`_. No partitioning is done by default. You can write your own partitioning function by implementing the interface for ``org.apache.pinot.query.spi.PartitionFunction``. The partitioning function can be configured on the required columns in the table config as

Review comment:
       `org.apache.pinot.segment.spi.partition.PartitionFunction` 




-- 
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



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


[GitHub] [incubator-pinot] mayankshriv merged pull request #6721: WIP: Pinot SPI refactor (no functionality change)

Posted by GitBox <gi...@apache.org>.
mayankshriv merged pull request #6721:
URL: https://github.com/apache/incubator-pinot/pull/6721


   


-- 
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



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