agavra commented on code in PR #9765:
URL: https://github.com/apache/pinot/pull/9765#discussion_r1018490355
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/QueryRunnerTestBase.java:
##########
@@ -198,11 +242,27 @@ private static List<String>
toH2FieldNamesAndTypes(org.apache.pinot.spi.data.Sch
case STRING:
fieldType = "varchar(128)";
break;
+ case DOUBLE:
+ fieldType = "double";
+ break;
default:
throw new UnsupportedOperationException("Unsupported type conversion
to h2 type: " + dataType);
}
fieldNamesAndTypes.add(fieldName + " " + fieldType);
}
return fieldNamesAndTypes;
}
+
+ public static class TestCase {
Review Comment:
{suggestion - feel free to disagree}
let's create a module named `model` and put all JSON classes in there. Also
let's not use maps and instead use additional models. For example:
```
class Tables {
@JsonProperty
Map<String, TableDefinition> _tables;
}
class TableDefinition {
@JsonProperty
List<ColumnAndType> _columns;
}
class ColumnAndType {
@JsonProperty
String name;
@JsonProperty
String type;
}
```
This is a bit of a pain up front but will make maintaining the test
framework much easier. One example of where this would be cleaner is that we
could have a rewriter utility that rewrites all table names to be properly
prefixed.
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/QueryRunnerTestBase.java:
##########
@@ -70,6 +60,128 @@ public class QueryRunnerTestBase extends QueryTestSet {
protected Map<ServerInstance, QueryServerEnclosure> _servers = new
HashMap<>();
protected GrpcMailboxService _mailboxService;
+ // --------------------------------------------------------------------------
+ // QUERY UTILS
+ // --------------------------------------------------------------------------
+ protected List<Object[]> queryRunner(String sql) {
+ QueryPlan queryPlan = _queryEnvironment.planQuery(sql);
+ Map<String, String> requestMetadataMap =
+ ImmutableMap.of("REQUEST_ID",
String.valueOf(RANDOM_REQUEST_ID_GEN.nextLong()));
+ MailboxReceiveOperator mailboxReceiveOperator = null;
+ for (int stageId : queryPlan.getStageMetadataMap().keySet()) {
+ if (queryPlan.getQueryStageMap().get(stageId) instanceof
MailboxReceiveNode) {
+ MailboxReceiveNode reduceNode = (MailboxReceiveNode)
queryPlan.getQueryStageMap().get(stageId);
+ mailboxReceiveOperator =
QueryDispatcher.createReduceStageOperator(_mailboxService,
+
queryPlan.getStageMetadataMap().get(reduceNode.getSenderStageId()).getServerInstances(),
+ Long.parseLong(requestMetadataMap.get("REQUEST_ID")),
reduceNode.getSenderStageId(),
+ reduceNode.getDataSchema(), "localhost", _reducerGrpcPort);
+ } else {
+ for (ServerInstance serverInstance :
queryPlan.getStageMetadataMap().get(stageId).getServerInstances()) {
+ DistributedStagePlan distributedStagePlan =
+ QueryDispatcher.constructDistributedStagePlan(queryPlan,
stageId, serverInstance);
+ _servers.get(serverInstance).processQuery(distributedStagePlan,
requestMetadataMap);
+ }
+ }
+ }
+ Preconditions.checkNotNull(mailboxReceiveOperator);
+ return
QueryDispatcher.toResultTable(QueryDispatcher.reduceMailboxReceive(mailboxReceiveOperator),
+ queryPlan.getQueryResultFields(),
queryPlan.getQueryStageMap().get(0).getDataSchema()).getRows();
+ }
+
+ protected List<Object[]> queryH2(String sql)
+ throws Exception {
+ Statement h2statement =
_h2Connection.createStatement(ResultSet.TYPE_FORWARD_ONLY,
ResultSet.CONCUR_READ_ONLY);
+ h2statement.execute(sql);
+ ResultSet h2ResultSet = h2statement.getResultSet();
+ int columnCount = h2ResultSet.getMetaData().getColumnCount();
+ List<Object[]> result = new ArrayList<>();
+ while (h2ResultSet.next()) {
+ Object[] row = new Object[columnCount];
+ for (int i = 0; i < columnCount; i++) {
+ row[i] = h2ResultSet.getObject(i + 1);
+ }
+ result.add(row);
+ }
+ return result;
+ }
+
+ protected void compareRowEquals(List<Object[]> resultRows, List<Object[]>
expectedRows) {
+ Assert.assertEquals(resultRows.size(), expectedRows.size());
+
+ Comparator<Object> valueComp = (l, r) -> {
+ if (l == null && r == null) {
+ return 0;
+ } else if (l == null) {
+ return -1;
+ } else if (r == null) {
+ return 1;
+ }
+ if (l instanceof Integer) {
+ return Integer.compare((Integer) l, ((Number) r).intValue());
+ } else if (l instanceof Long) {
+ return Long.compare((Long) l, ((Number) r).longValue());
+ } else if (l instanceof Float) {
+ return Float.compare((Float) l, ((Number) r).floatValue());
+ } else if (l instanceof Double) {
+ return Double.compare((Double) l, ((Number) r).doubleValue());
+ } else if (l instanceof String) {
+ return ((String) l).compareTo((String) r);
+ } else if (l instanceof Boolean) {
+ return ((Boolean) l).compareTo((Boolean) r);
+ } else {
+ throw new RuntimeException("non supported type " + l.getClass());
+ }
+ };
+ Comparator<Object[]> rowComp = (l, r) -> {
+ int cmp = 0;
+ for (int i = 0; i < l.length; i++) {
+ cmp = valueComp.compare(l[i], r[i]);
+ if (cmp != 0) {
+ return cmp;
+ }
+ }
+ return 0;
+ };
+ resultRows.sort(rowComp);
+ expectedRows.sort(rowComp);
+ for (int i = 0; i < resultRows.size(); i++) {
+ Object[] resultRow = resultRows.get(i);
+ Object[] expectedRow = expectedRows.get(i);
+ for (int j = 0; j < resultRow.length; j++) {
+ Assert.assertEquals(valueComp.compare(resultRow[j], expectedRow[j]), 0,
+ "Not match at (" + i + "," + j + ")! Expected: " + expectedRow[j]
+ " Actual: " + resultRow[j]);
+ }
+ }
+ }
+
+ // --------------------------------------------------------------------------
+ // TEST CASES PREP
+ // --------------------------------------------------------------------------
+ protected Schema constructSchema(String schemaName, Map<String, String>
dataTypes) {
+ Schema.SchemaBuilder builder = new Schema.SchemaBuilder();
+ for (Map.Entry<String, String> dataType : dataTypes.entrySet()) {
+ builder.addSingleValueDimension(dataType.getKey(),
FieldSpec.DataType.valueOf(dataType.getValue()));
+ }
+ // ts is built in
+ builder.addDateTime("ts", FieldSpec.DataType.LONG, "1:MILLISECONDS:EPOCH",
"1:HOURS");
+ builder.setSchemaName(schemaName);
+ return builder.build();
+ }
+
+ protected List<GenericRow> toRow(List<List<Object>> value) {
+ List<GenericRow> result = new ArrayList<>(value.size());
+ for (int rowId = 0; rowId < value.size(); rowId++) {
+ GenericRow row = new GenericRow();
+ List<Object> rawRow = value.get(rowId);
+ for (int colId = 0; colId < rawRow.size(); colId++) {
+ row.putValue("col" + colId, rawRow.get(colId));
+ }
+ row.putValue("ts", System.currentTimeMillis());
Review Comment:
if `ts` is in the row we should use that instead (i.e. make sure we can
specify it in the json file)
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/queries/QueryTestExecutor.java:
##########
@@ -0,0 +1,171 @@
+/**
+ * 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.query.runtime.queries;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.ImmutableList;
+import java.io.File;
+import java.net.URL;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import org.apache.pinot.common.datatable.DataTableFactory;
+import org.apache.pinot.core.common.datatable.DataTableBuilderFactory;
+import org.apache.pinot.query.QueryEnvironmentTestBase;
+import org.apache.pinot.query.QueryServerEnclosure;
+import org.apache.pinot.query.mailbox.GrpcMailboxService;
+import org.apache.pinot.query.routing.WorkerInstance;
+import org.apache.pinot.query.runtime.QueryRunnerTestBase;
+import org.apache.pinot.query.service.QueryConfig;
+import org.apache.pinot.query.testutils.MockInstanceDataManagerFactory;
+import org.apache.pinot.query.testutils.QueryTestUtils;
+import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.data.Schema;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.builder.TableNameBuilder;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+
+public class QueryTestExecutor extends QueryRunnerTestBase {
+ private static final ObjectMapper MAPPER = new ObjectMapper();
+ private static final Pattern TABLE_NAME_REPLACE_PATTERN =
Pattern.compile("\\{([\\w\\d]+)\\}");
+ private static final String QUERY_TEST_RESOURCE_FOLDER = "queries";
+ private static final List<String> QUERY_TEST_RESOURCE_FILES =
ImmutableList.of(
+ "BasicQuery.json"
+ );
Review Comment:
we should just load all test resources dynamically - I can do that in a
follow up, we don't need to block this PR on it
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/queries/QueryTestExecutor.java:
##########
@@ -0,0 +1,171 @@
+/**
+ * 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.query.runtime.queries;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.ImmutableList;
+import java.io.File;
+import java.net.URL;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import org.apache.pinot.common.datatable.DataTableFactory;
+import org.apache.pinot.core.common.datatable.DataTableBuilderFactory;
+import org.apache.pinot.query.QueryEnvironmentTestBase;
+import org.apache.pinot.query.QueryServerEnclosure;
+import org.apache.pinot.query.mailbox.GrpcMailboxService;
+import org.apache.pinot.query.routing.WorkerInstance;
+import org.apache.pinot.query.runtime.QueryRunnerTestBase;
+import org.apache.pinot.query.service.QueryConfig;
+import org.apache.pinot.query.testutils.MockInstanceDataManagerFactory;
+import org.apache.pinot.query.testutils.QueryTestUtils;
+import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.data.Schema;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.builder.TableNameBuilder;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+
+public class QueryTestExecutor extends QueryRunnerTestBase {
+ private static final ObjectMapper MAPPER = new ObjectMapper();
+ private static final Pattern TABLE_NAME_REPLACE_PATTERN =
Pattern.compile("\\{([\\w\\d]+)\\}");
+ private static final String QUERY_TEST_RESOURCE_FOLDER = "queries";
+ private static final List<String> QUERY_TEST_RESOURCE_FILES =
ImmutableList.of(
+ "BasicQuery.json"
+ );
+
+ @BeforeClass
+ public void setUp()
+ throws Exception {
+ DataTableBuilderFactory.setDataTableVersion(DataTableFactory.VERSION_4);
+ // Setting up mock server factories.
+ MockInstanceDataManagerFactory factory1 = new
MockInstanceDataManagerFactory("server1");
+ MockInstanceDataManagerFactory factory2 = new
MockInstanceDataManagerFactory("server2");
+ // Setting up H2 for validation
+ setH2Connection();
+
+ // Scan through all the test cases.
+ for (Map.Entry<String, TestCase> testCaseEntry :
getTestCases().entrySet()) {
Review Comment:
why do this in `@BeforeClass` instead of on every test? (e.g. set up the H2
connection once, but actually register the tables on each test)
##########
pinot-query-runtime/src/test/resources/queries/BasicQuery.json:
##########
@@ -0,0 +1,39 @@
+
+{
+ "basic_test": {
+ "description": "basic test case example",
+ "sql": "SELECT * FROM {tbl}",
+ "tables": {
+ "tbl": { "col1": "STRING", "col2": "INT" }
Review Comment:
since order matters in the `inputs` part of the model, we should model this
part as an array of tuples (as suggested in comment above) instead of a map
since JSON can reorder maps if it wants:
```
"tbl": [{"name":"col1", "type":"STRING}, {"name":"col2", "type":"INT"}]
```
Let's commit this first, I think I might be able to write a simple utility
that allows us to declare tables using SQL syntax and then turn it into what we
want:
```
"tbl": "col1 STRING, col2 INT"
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
