[GitHub] [pinot] walterddr commented on a change in pull request #8408: Combine Metadata and Dictionary based plans into single operator

[GitBox <gi...@apache.org>]

walterddr commented on a change in pull request #8408:
URL: https://github.com/apache/pinot/pull/8408#discussion_r837693392



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/query/NonScanBasedAggregationOperator.java
##########
@@ -122,8 +131,26 @@ protected IntermediateResultsBlock getNextBlock() {
     return new IntermediateResultsBlock(_aggregationFunctions, aggregationResults, false);
   }
 
-  private double toDouble(Comparable value) {
-    if (value instanceof Number) {
+  private static Double getMinValue(DataSource dataSource) {
+    Dictionary dictionary = dataSource.getDictionary();
+    if (dictionary != null) {
+      return toDouble(dictionary.getMinVal());
+    }
+    return toDouble(dataSource.getDataSourceMetadata().getMinValue());

Review comment:
       dataSource.getDictionary() seems final to me? can we optimized out this if check?

##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MetadataAndDictionaryAggregationPlanClusterIntegrationTest.java
##########
@@ -242,10 +241,10 @@ public void testDictionaryBasedQueries()
     assertEquals(response.get("numEntriesScannedInFilter").asLong(), 0);
     assertEquals(response.get("totalDocs").asLong(), response.get("numDocsScanned").asLong());
 
-    // Non dictionary column: not answered by DictionaryBasedAggregationOperator
+    // Non dictionary column: answered by MetadataBasedAggregationOperator

Review comment:
       ```suggestion
       // Non dictionary column: answered by NonScanBasedAggregationOperator
   ```

##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MetadataAndDictionaryAggregationPlanClusterIntegrationTest.java
##########
@@ -242,10 +241,10 @@ public void testDictionaryBasedQueries()
     assertEquals(response.get("numEntriesScannedInFilter").asLong(), 0);
     assertEquals(response.get("totalDocs").asLong(), response.get("numDocsScanned").asLong());
 
-    // Non dictionary column: not answered by DictionaryBasedAggregationOperator
+    // Non dictionary column: answered by MetadataBasedAggregationOperator

Review comment:
       and similar applies to other 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.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

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