[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #4833: Use ResultTable for all queries

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

mayankshriv commented on a change in pull request #4833: Use ResultTable for all queries
URL: https://github.com/apache/incubator-pinot/pull/4833#discussion_r348077129
 
 

 ##########
 File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/AggregationResultSetter.java
 ##########
 @@ -0,0 +1,253 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.reduce;
+
+import com.google.common.base.Preconditions;
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import org.apache.pinot.common.function.AggregationFunctionType;
+import org.apache.pinot.common.metrics.BrokerMetrics;
+import org.apache.pinot.common.request.AggregationInfo;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.response.broker.AggregationResult;
+import org.apache.pinot.common.response.broker.BrokerResponseNative;
+import org.apache.pinot.common.response.broker.ResultTable;
+import org.apache.pinot.common.response.broker.SelectionResults;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.common.utils.DataTable;
+import org.apache.pinot.core.data.table.Key;
+import org.apache.pinot.core.query.aggregation.DistinctTable;
+import org.apache.pinot.core.query.aggregation.function.AggregationFunction;
+import org.apache.pinot.core.query.aggregation.function.AggregationFunctionUtils;
+import org.apache.pinot.core.transport.ServerRoutingInstance;
+
+
+/**
+ * Helper class to set Aggregation results into the BrokerResponseNative
+ * Also handles Distinct results
+ */
+public class AggregationResultSetter extends ResultSetter {
 
 Review comment:
   The ResultSetter class hierarchy seems a bit confusing. The base class just seems to have a constructor (i.e. no methods to overwrite). Also, there's a special casing for Distinct inside the setter. Will the following work:
   1. Create an interface ResultSetter with api `setResults`. We can also create an abstract base class if there's enough common code.
   2. Create implementations of this interface for Selection and Aggregation.
   3. Create a factory that returns the right result setter based on aggregation function and response format.
   4. For Distinct, the factory can return the selection setter.
   
   This way, all of the conditional logic is inside the factory, and the appropriate setter is hooked up to do its job.
   
   How about if we have a ResultSetterFactory that returns the appropriate result setter based on response format and aggregation function. This also allows to handle the Distinct case to be modeled as selection result.

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