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/10/23 08:11:30 UTC

[GitHub] [incubator-pinot] jackjlli opened a new pull request #6181: Reduce pql queries from 10k to 500 in pinot-integration-tests module

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


   ## Description
   This PR:
   * removes all the queries that return empty result.
   * reduces pql queries from 10k to 500 in pinot-integration-tests 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] sajjad-moradi commented on a change in pull request #6181: Reduce pql queries from 10k to 500 in pinot-integration-tests module

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



##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTestSet.java
##########
@@ -320,19 +320,17 @@ public void testQueriesFromQueryFile()
     assertNotNull(resourceUrl);
     File queryFile = new File(resourceUrl.getFile());
 
-    int maxNumQueriesToSkipInQueryFile = getMaxNumQueriesToSkipInQueryFile();
     try (BufferedReader reader = new BufferedReader(new FileReader(queryFile))) {
       while (true) {
-        int numQueriesSkipped = RANDOM.nextInt(maxNumQueriesToSkipInQueryFile);
-        for (int i = 0; i < numQueriesSkipped; i++) {
-          reader.readLine();
-        }
-
         String queryString = reader.readLine();
         // Reach end of file.
         if (queryString == null) {
           return;
         }
+        // Skip commented line and empty line.
+        if (queryString.startsWith("#") || queryString.isEmpty()) {

Review comment:
       Use `queryString.trim()` to include leading whitespaces before # or empty lines with only whitespaces.




----------------------------------------------------------------
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 #6181: Reduce pql queries from 10k to 500 in pinot-integration-tests module

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


   @jackjlli Slightly tangential question, most users are either migrating to or already using SQL. Is there a SQL version these queries that we are validating against?


----------------------------------------------------------------
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] jackjlli commented on a change in pull request #6181: Reduce pql queries from 10k to 500 in pinot-integration-tests module

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



##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTestSet.java
##########
@@ -320,19 +320,18 @@ public void testQueriesFromQueryFile()
     assertNotNull(resourceUrl);
     File queryFile = new File(resourceUrl.getFile());
 
-    int maxNumQueriesToSkipInQueryFile = getMaxNumQueriesToSkipInQueryFile();
     try (BufferedReader reader = new BufferedReader(new FileReader(queryFile))) {
       while (true) {

Review comment:
       Updated.




----------------------------------------------------------------
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] jackjlli commented on pull request #6181: Reduce pql queries from 10k to 500 in pinot-integration-tests module

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


   > @jackjlli Slightly tangential question, most users are either migrating to or already using SQL. Is there a SQL version these queries that we are validating against?
   
   Yes, there will be a PR for SQL queries as well after this one.


----------------------------------------------------------------
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] jackjlli merged pull request #6181: Reduce pql queries from 10k to 500 in pinot-integration-tests module

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


   


----------------------------------------------------------------
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 #6181: Reduce pql queries from 10k to 500 in pinot-integration-tests module

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


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6181?src=pr&el=h1) Report
   > Merging [#6181](https://codecov.io/gh/apache/incubator-pinot/pull/6181?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) will **increase** coverage by `6.74%`.
   > The diff coverage is `60.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/6181/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/6181?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #6181      +/-   ##
   ==========================================
   + Coverage   66.44%   73.19%   +6.74%     
   ==========================================
     Files        1075     1236     +161     
     Lines       54773    58469    +3696     
     Branches     8168     8653     +485     
   ==========================================
   + Hits        36396    42796    +6400     
   + Misses      15700    12869    -2831     
   - Partials     2677     2804     +127     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #integration | `46.31% <49.14%> (?)` | |
   | #unittests | `64.07% <38.09%> (?)` | |
   
   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/6181?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6181/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6181/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
   | [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6181/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `53.33% <0.00%> (-3.81%)` | :arrow_down: |
   | [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6181/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `44.44% <0.00%> (-4.40%)` | :arrow_down: |
   | [.../org/apache/pinot/client/ResultTableResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6181/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L1Jlc3VsdFRhYmxlUmVzdWx0U2V0LmphdmE=) | `24.00% <0.00%> (-10.29%)` | :arrow_down: |
   | [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6181/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `78.57% <ø> (+5.40%)` | :arrow_up: |
   | [.../apache/pinot/common/exception/QueryException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6181/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZXhjZXB0aW9uL1F1ZXJ5RXhjZXB0aW9uLmphdmE=) | `90.27% <ø> (+5.55%)` | :arrow_up: |
   | [...pinot/common/function/AggregationFunctionType.java](https://codecov.io/gh/apache/incubator-pinot/pull/6181/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vQWdncmVnYXRpb25GdW5jdGlvblR5cGUuamF2YQ==) | `98.27% <ø> (-1.73%)` | :arrow_down: |
   | [.../pinot/common/function/DateTimePatternHandler.java](https://codecov.io/gh/apache/incubator-pinot/pull/6181/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRGF0ZVRpbWVQYXR0ZXJuSGFuZGxlci5qYXZh) | `83.33% <ø> (ø)` | |
   | [...ot/common/function/FunctionDefinitionRegistry.java](https://codecov.io/gh/apache/incubator-pinot/pull/6181/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRnVuY3Rpb25EZWZpbml0aW9uUmVnaXN0cnkuamF2YQ==) | `88.88% <ø> (+44.44%)` | :arrow_up: |
   | ... and [1007 more](https://codecov.io/gh/apache/incubator-pinot/pull/6181/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6181?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/6181?src=pr&el=footer). Last update [2484f5b...6aa17fd](https://codecov.io/gh/apache/incubator-pinot/pull/6181?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] jackjlli commented on pull request #6181: Reduce pql queries from 10k to 500 in pinot-integration-tests module

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


   @Jackie-Jiang Updated the pql query file.


----------------------------------------------------------------
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] jackjlli commented on a change in pull request #6181: Reduce pql queries from 10k to 500 in pinot-integration-tests module

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



##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTestSet.java
##########
@@ -60,7 +60,7 @@
 
   // Default settings
   private static final String DEFAULT_PQL_QUERY_FILE_NAME =
-      "On_Time_On_Time_Performance_2014_100k_subset.test_queries_10K";
+      "On_Time_On_Time_Performance_2014_100k_subset.test_queries_1K";

Review comment:
       Changed.




----------------------------------------------------------------
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] jackjlli commented on pull request #6181: Reduce pql queries from 10k to 500 in pinot-integration-tests module

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


   > You may want to add a couple of queries that return empty results also.
   
   Not necessarily, the queries with empty response will skip the comparison of the results like below:
   https://github.com/apache/incubator-pinot/blob/f2a990bdca52f2227b64e3629435b28e6a800f3a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/ClusterIntegrationTestUtils.java#L527


----------------------------------------------------------------
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 #6181: Reduce pql queries from 10k to 500 in pinot-integration-tests module

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


   Do we have a coverage report for this one? Granted that queries with empty results are not effective, but if the code path is still exercised and we see a drop in coverage, we may want to substitute these with new queries that bring the coverage back up.


----------------------------------------------------------------
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 #6181: Reduce pql queries from 10k to 500 in pinot-integration-tests module

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



##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTestSet.java
##########
@@ -60,7 +60,7 @@
 
   // Default settings
   private static final String DEFAULT_PQL_QUERY_FILE_NAME =
-      "On_Time_On_Time_Performance_2014_100k_subset.test_queries_10K";
+      "On_Time_On_Time_Performance_2014_100k_subset.test_queries_1K";

Review comment:
       Correctly reflect the number of queries?
   ```suggestion
         "On_Time_On_Time_Performance_2014_100k_subset.test_queries_500";
   ```

##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTestSet.java
##########
@@ -320,19 +320,18 @@ public void testQueriesFromQueryFile()
     assertNotNull(resourceUrl);
     File queryFile = new File(resourceUrl.getFile());
 
-    int maxNumQueriesToSkipInQueryFile = getMaxNumQueriesToSkipInQueryFile();
     try (BufferedReader reader = new BufferedReader(new FileReader(queryFile))) {
       while (true) {

Review comment:
       (nit)
   ```suggestion
         String queryString;
         while ((queryString = reader.readLine()) != null) {
   ```




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