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/31 01:04:31 UTC

[GitHub] [incubator-pinot] sajjad-moradi opened a new pull request #6731: Fix an issue with datetime column in Rule Engine

sajjad-moradi opened a new pull request #6731:
URL: https://github.com/apache/incubator-pinot/pull/6731


   ## Description
   With the fix, Rule Engine now works with both `time` and `dateTime` types. Previously it used to only work for time column.
   
   ## Testing Done
   - added a unit test for one of the rules to have dateTime column instead of time column
   - locally changed the inputs of all the rules in the unit tests to have dateTime column instead of time column and they all passed. Didn't want to duplicate all the unit tests - one for time column and one for dateTime column - so I didn't include them in the PR. One unit test with dateTime column should suffice.
   


-- 
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] siddharthteotia commented on a change in pull request #6731: Fix an issue with datetime column in Rule Engine

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



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/recommender/io/InputManager.java
##########
@@ -181,15 +182,19 @@ private void validateQueries() {
     invalidQueries.forEach(_queryWeightMap::remove);
   }
 
-  public void registerColnameFieldType() { // create a map from colname to data type
+  public void registerColNameFieldType() { // create a map from col name to data type

Review comment:
       Not related to the changes in this PR but can we do the following cleanup ?
   - Make this private. It is not used elsewhere.
   - Move the javadoc to one line before. 




-- 
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] siddharthteotia commented on a change in pull request #6731: Fix an issue with datetime column in Rule Engine

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



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/recommender/io/InputManager.java
##########
@@ -181,15 +182,19 @@ private void validateQueries() {
     invalidQueries.forEach(_queryWeightMap::remove);
   }
 
-  public void registerColnameFieldType() { // create a map from colname to data type
+  public void registerColNameFieldType() { // create a map from col name to data type

Review comment:
       still shows public ?




-- 
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] sajjad-moradi commented on a change in pull request #6731: Fix an issue with datetime column in Rule Engine

Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #6731:
URL: https://github.com/apache/incubator-pinot/pull/6731#discussion_r605339518



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/recommender/io/InputManager.java
##########
@@ -181,15 +182,19 @@ private void validateQueries() {
     invalidQueries.forEach(_queryWeightMap::remove);
   }
 
-  public void registerColnameFieldType() { // create a map from colname to data type
+  public void registerColNameFieldType() { // create a map from col name to data type

Review comment:
       Sure. 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] siddharthteotia merged pull request #6731: Fix an issue with datetime column in Rule Engine

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


   


-- 
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] sajjad-moradi commented on a change in pull request #6731: Fix an issue with datetime column in Rule Engine

Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #6731:
URL: https://github.com/apache/incubator-pinot/pull/6731#discussion_r605379645



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/recommender/io/InputManager.java
##########
@@ -181,15 +182,19 @@ private void validateQueries() {
     invalidQueries.forEach(_queryWeightMap::remove);
   }
 
-  public void registerColnameFieldType() { // create a map from colname to data type
+  public void registerColNameFieldType() { // create a map from col name to data type

Review comment:
       😅 
   Done 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] codecov-io commented on pull request #6731: Fix an issue with datetime column in Rule Engine

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


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6731?src=pr&el=h1) Report
   > Merging [#6731](https://codecov.io/gh/apache/incubator-pinot/pull/6731?src=pr&el=desc) (7a80670) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/d77a8d8248ab6e0e8f7a55621dcf9cf6374c05c6?el=desc) (d77a8d8) will **decrease** coverage by `7.76%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/6731/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/6731?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #6731      +/-   ##
   ============================================
   - Coverage     73.81%   66.04%   -7.77%     
     Complexity       12       12              
   ============================================
     Files          1402     1402              
     Lines         68042    68047       +5     
     Branches       9828     9831       +3     
   ============================================
   - Hits          50227    44945    -5282     
   - Misses        14559    19924    +5365     
   + Partials       3256     3178      -78     
   ```
   
   | Flag | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | integration | `?` | `?` | |
   | unittests | `66.04% <100.00%> (+0.02%)` | `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/6731?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [.../pinot/controller/recommender/io/InputManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/6731/diff?src=pr&el=tree#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9pby9JbnB1dE1hbmFnZXIuamF2YQ==) | `85.39% <100.00%> (+2.18%)` | `0.00 <0.00> (ø)` | |
   | [...a/org/apache/pinot/minion/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6731/diff?src=pr&el=tree#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
   | [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6731/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
   | [.../apache/pinot/minion/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6731/diff?src=pr&el=tree#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
   | [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://codecov.io/gh/apache/incubator-pinot/pull/6731/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUXVlcnlDbGllbnQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
   | [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6731/diff?src=pr&el=tree#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
   | [...t/core/startree/plan/StarTreeDocIdSetPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6731/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlRG9jSWRTZXRQbGFuTm9kZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
   | [.../core/startree/plan/StarTreeTransformPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6731/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlVHJhbnNmb3JtUGxhbk5vZGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
   | [...core/startree/plan/StarTreeProjectionPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6731/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlUHJvamVjdGlvblBsYW5Ob2RlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
   | [...t/minion/executor/MinionTaskZkMetadataManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/6731/diff?src=pr&el=tree#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhlY3V0b3IvTWluaW9uVGFza1prTWV0YWRhdGFNYW5hZ2VyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
   | ... and [338 more](https://codecov.io/gh/apache/incubator-pinot/pull/6731/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6731?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/6731?src=pr&el=footer). Last update [d77a8d8...7a80670](https://codecov.io/gh/apache/incubator-pinot/pull/6731?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