snleee commented on a change in pull request #6224:
URL: https://github.com/apache/incubator-pinot/pull/6224#discussion_r521590779
##########
File path:
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/ClusterIntegrationTestUtils.java
##########
@@ -975,6 +831,197 @@ private static boolean isSelectionQuery(BrokerRequest
brokerRequest) {
return false;
}
+ private static void convertToUpperCase(List<String> columns) {
+ for (int i = 0; i < columns.size(); i++) {
+ columns.set(i, columns.get(i).toUpperCase());
+ }
+ }
+
+ private static int getH2ExpectedValues(Set<String> expectedValues,
List<String> expectedOrderByValues,
+ ResultSet h2ResultSet, ResultSetMetaData h2MetaData, Collection<String>
orderByColumns) throws SQLException {
+ Map<String, String> reusableExpectedValueMap = new HashMap<>();
+ Map<String, List<String>> reusableMultiValuesMap = new HashMap<>();
+ List<String> reusableColumnOrder = new ArrayList<>();
+ int h2NumRows;
+ int numColumns = h2MetaData.getColumnCount();
+
+ for (h2NumRows = 0; h2ResultSet.next() && h2NumRows <
MAX_NUM_ROWS_TO_COMPARE; h2NumRows++) {
+ reusableExpectedValueMap.clear();
+ reusableMultiValuesMap.clear();
+ reusableColumnOrder.clear();
+
+ for (int columnIndex = 1; columnIndex <= numColumns; columnIndex++) { //
h2 result set is 1-based
+ String columnName = h2MetaData.getColumnName(columnIndex);
+
+ // Handle null result and convert boolean value to lower case
+ String columnValue = h2ResultSet.getString(columnIndex);
+ if (columnValue == null) {
+ columnValue = "null";
+ } else {
+ columnValue = convertBooleanToLowerCase(columnValue);
+ }
+
+ // Handle multi-value columns
+ int length = columnName.length();
+ if (length > 5 && columnName.substring(length - 5, length -
1).equals("__MV")) {
Review comment:
What does this `5` means?
##########
File path:
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/ClusterIntegrationTestUtils.java
##########
@@ -975,6 +831,197 @@ private static boolean isSelectionQuery(BrokerRequest
brokerRequest) {
return false;
}
+ private static void convertToUpperCase(List<String> columns) {
+ for (int i = 0; i < columns.size(); i++) {
+ columns.set(i, columns.get(i).toUpperCase());
+ }
+ }
+
+ private static int getH2ExpectedValues(Set<String> expectedValues,
List<String> expectedOrderByValues,
+ ResultSet h2ResultSet, ResultSetMetaData h2MetaData, Collection<String>
orderByColumns) throws SQLException {
+ Map<String, String> reusableExpectedValueMap = new HashMap<>();
+ Map<String, List<String>> reusableMultiValuesMap = new HashMap<>();
+ List<String> reusableColumnOrder = new ArrayList<>();
+ int h2NumRows;
+ int numColumns = h2MetaData.getColumnCount();
+
+ for (h2NumRows = 0; h2ResultSet.next() && h2NumRows <
MAX_NUM_ROWS_TO_COMPARE; h2NumRows++) {
+ reusableExpectedValueMap.clear();
+ reusableMultiValuesMap.clear();
+ reusableColumnOrder.clear();
+
+ for (int columnIndex = 1; columnIndex <= numColumns; columnIndex++) { //
h2 result set is 1-based
+ String columnName = h2MetaData.getColumnName(columnIndex);
+
+ // Handle null result and convert boolean value to lower case
+ String columnValue = h2ResultSet.getString(columnIndex);
+ if (columnValue == null) {
+ columnValue = "null";
+ } else {
+ columnValue = convertBooleanToLowerCase(columnValue);
+ }
+
+ // Handle multi-value columns
+ int length = columnName.length();
+ if (length > 5 && columnName.substring(length - 5, length -
1).equals("__MV")) {
+ // Multi-value column
+ String multiValueColumnName = columnName.substring(0, length - 5);
+ List<String> multiValue =
reusableMultiValuesMap.get(multiValueColumnName);
+ if (multiValue == null) {
+ multiValue = new ArrayList<>();
+ reusableMultiValuesMap.put(multiValueColumnName, multiValue);
+ reusableColumnOrder.add(multiValueColumnName);
+ }
+ multiValue.add(columnValue);
+ } else {
+ // Single-value column
+ String columnDataType = h2MetaData.getColumnTypeName(columnIndex);
+ columnValue = removeTrailingZeroForNumber(columnValue,
columnDataType);
+ reusableExpectedValueMap.put(columnName, columnValue);
+ reusableColumnOrder.add(columnName);
+ }
+ }
+
+ // Add multi-value column results to the expected values
+ // The reason for this step is that Pinot does not maintain order of
elements in multi-value columns
+ for (Map.Entry<String, List<String>> entry :
reusableMultiValuesMap.entrySet()) {
+ List<String> multiValue = entry.getValue();
+ Collections.sort(multiValue);
+ reusableExpectedValueMap.put(entry.getKey(), multiValue.toString());
+ }
+
+ // Build expected value String
+ StringBuilder expectedValue = new StringBuilder();
+ StringBuilder expectedOrderByValue = new StringBuilder();
+ for (String column : reusableColumnOrder) {
+ expectedValue.append(reusableExpectedValueMap.get(column)).append(' ');
+ if (orderByColumns.contains(column)) {
+
expectedOrderByValue.append(reusableExpectedValueMap.get(column)).append(' ');
+ }
+ }
+ expectedValues.add(expectedValue.toString());
+ expectedOrderByValues.add(expectedOrderByValue.toString());
+ }
+
+ return h2NumRows;
+ }
+
+ private static void comparePinotResultsWithExpectedValues(Set<String>
expectedValues, List<String> expectedOrderByValues,
+ org.apache.pinot.client.ResultSet connectionResultSet,
Collection<String> orderByColumns, String pinotQuery, List<String> sqlQueries,
+ int h2NumRows, long pinotNumRecordsSelected) throws IOException,
SQLException {
+
+ int pinotNumRows = connectionResultSet.getRowCount();
+ // No record selected in H2
+ if (h2NumRows== 0) {
+ if (pinotNumRows != 0) {
+ String failureMessage = "No record selected in H2 but number of
records selected in Pinot: " + pinotNumRows;
+ failure(pinotQuery, sqlQueries, failureMessage);
+ return;
+ }
+
+ if (pinotNumRecordsSelected != 0) {
+ String failureMessage =
+ "No selection result returned in Pinot but number of records
selected: " + pinotNumRecordsSelected;
+ failure(pinotQuery, sqlQueries, failureMessage);
+ return;
+ }
+
+ // Skip further comparison
+ return;
+ }
+
+ PinotQuery compiledQuery =
CalciteSqlParser.compileToPinotQuery(pinotQuery);
+ boolean isLimitSet = compiledQuery.isSetLimit();
+ int limit = compiledQuery.getLimit();
+
+ // Only compare exhausted results
+ if (h2NumRows < MAX_NUM_ROWS_TO_COMPARE) {
+
+ for (int rowIndex = 0; rowIndex < pinotNumRows; rowIndex++) {
+ // Build actual value String.
+ StringBuilder actualValueBuilder = new StringBuilder();
+ StringBuilder actualOrderByValueBuilder = new StringBuilder();
+ for (int columnIndex = 0; columnIndex <
connectionResultSet.getColumnCount(); columnIndex++) {
+ // Convert column name to all uppercase to make it compatible with H2
+ String columnName =
connectionResultSet.getColumnName(columnIndex).toUpperCase();
+ String columnResult = connectionResultSet.getString(rowIndex,
columnIndex);
+
+ String columnDataType =
connectionResultSet.getColumnDataType(columnIndex);
+ columnResult = removeTrailingZeroForNumber(columnResult,
columnDataType);
+ // TODO: Find a better way to identify multi-value column
+ if (columnResult.charAt(0) == '[') {
Review comment:
If you check the `connectionResultSet.getString()`, we are essentially
converting `JsonNode` to string. We can read the string back to json node and
figure out whether it's array or not.
##########
File path:
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/ClusterIntegrationTestUtils.java
##########
@@ -833,6 +700,33 @@ static void testSqlQuery(String pinotQuery, String
brokerUrl, org.apache.pinot.c
return;
}
+ BrokerRequest brokerRequest =
+
PinotQueryParserFactory.get(CommonConstants.Broker.Request.SQL).compileToBrokerRequest(pinotQuery);
+ // Add order by columns which are not in selection clause for comparison
purpose
+ List<String> orderByColumns = new ArrayList<>();
+ List<String> selectionColumns = new ArrayList<>();
+ if (isSelectionQuery(brokerRequest) && brokerRequest.getOrderBy() != null
&& brokerRequest.getOrderBy().size() > 0) {
+
orderByColumns.addAll(CalciteSqlParser.extractIdentifiers(brokerRequest.getPinotQuery().getOrderByList(),
false));
+
selectionColumns.addAll(CalciteSqlParser.extractIdentifiers(brokerRequest.getPinotQuery().getSelectList(),
false));
+ convertToUpperCase(orderByColumns);
+ convertToUpperCase(selectionColumns);
+
+ if (!selectionColumns.containsAll(orderByColumns)) {
+ List<String> inputRequests = new ArrayList<>();
+ inputRequests.add(pinotQuery);
+ inputRequests.addAll(sqlQueries);
+
+ Set<String> orderByColumnsExcluded = new HashSet<>(orderByColumns);
+ orderByColumnsExcluded.removeAll(selectionColumns);
+
+ // Append order-by columns not in selection clause so the order of
query responses can be verified
+ // e.g. we can't verify the order of query `SELECT firstName FROM
mytable ORDER BY lastName' if there are duplicate lastNames
Review comment:
Can you post the sample query that you are trying to solve from order-by
clause append?
IMO, the queries from the sample set should have a deterministic result
instead of implicitly manipulating the order by clause here.
We cannot do the result comparison check if the query result by definition
can be changed (e.g. selection with no order by)
##########
File path:
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/ClusterIntegrationTestUtils.java
##########
@@ -975,6 +831,197 @@ private static boolean isSelectionQuery(BrokerRequest
brokerRequest) {
return false;
}
+ private static void convertToUpperCase(List<String> columns) {
+ for (int i = 0; i < columns.size(); i++) {
+ columns.set(i, columns.get(i).toUpperCase());
+ }
+ }
+
+ private static int getH2ExpectedValues(Set<String> expectedValues,
List<String> expectedOrderByValues,
+ ResultSet h2ResultSet, ResultSetMetaData h2MetaData, Collection<String>
orderByColumns) throws SQLException {
+ Map<String, String> reusableExpectedValueMap = new HashMap<>();
+ Map<String, List<String>> reusableMultiValuesMap = new HashMap<>();
+ List<String> reusableColumnOrder = new ArrayList<>();
+ int h2NumRows;
+ int numColumns = h2MetaData.getColumnCount();
+
+ for (h2NumRows = 0; h2ResultSet.next() && h2NumRows <
MAX_NUM_ROWS_TO_COMPARE; h2NumRows++) {
+ reusableExpectedValueMap.clear();
+ reusableMultiValuesMap.clear();
+ reusableColumnOrder.clear();
+
+ for (int columnIndex = 1; columnIndex <= numColumns; columnIndex++) { //
h2 result set is 1-based
+ String columnName = h2MetaData.getColumnName(columnIndex);
+
+ // Handle null result and convert boolean value to lower case
+ String columnValue = h2ResultSet.getString(columnIndex);
+ if (columnValue == null) {
+ columnValue = "null";
+ } else {
+ columnValue = convertBooleanToLowerCase(columnValue);
+ }
+
+ // Handle multi-value columns
+ int length = columnName.length();
+ if (length > 5 && columnName.substring(length - 5, length -
1).equals("__MV")) {
+ // Multi-value column
+ String multiValueColumnName = columnName.substring(0, length - 5);
+ List<String> multiValue =
reusableMultiValuesMap.get(multiValueColumnName);
+ if (multiValue == null) {
+ multiValue = new ArrayList<>();
+ reusableMultiValuesMap.put(multiValueColumnName, multiValue);
+ reusableColumnOrder.add(multiValueColumnName);
+ }
+ multiValue.add(columnValue);
+ } else {
+ // Single-value column
+ String columnDataType = h2MetaData.getColumnTypeName(columnIndex);
+ columnValue = removeTrailingZeroForNumber(columnValue,
columnDataType);
+ reusableExpectedValueMap.put(columnName, columnValue);
+ reusableColumnOrder.add(columnName);
+ }
+ }
+
+ // Add multi-value column results to the expected values
+ // The reason for this step is that Pinot does not maintain order of
elements in multi-value columns
+ for (Map.Entry<String, List<String>> entry :
reusableMultiValuesMap.entrySet()) {
+ List<String> multiValue = entry.getValue();
+ Collections.sort(multiValue);
+ reusableExpectedValueMap.put(entry.getKey(), multiValue.toString());
+ }
+
+ // Build expected value String
+ StringBuilder expectedValue = new StringBuilder();
+ StringBuilder expectedOrderByValue = new StringBuilder();
+ for (String column : reusableColumnOrder) {
+ expectedValue.append(reusableExpectedValueMap.get(column)).append(' ');
+ if (orderByColumns.contains(column)) {
+
expectedOrderByValue.append(reusableExpectedValueMap.get(column)).append(' ');
+ }
+ }
+ expectedValues.add(expectedValue.toString());
+ expectedOrderByValues.add(expectedOrderByValue.toString());
+ }
+
+ return h2NumRows;
+ }
+
+ private static void comparePinotResultsWithExpectedValues(Set<String>
expectedValues, List<String> expectedOrderByValues,
+ org.apache.pinot.client.ResultSet connectionResultSet,
Collection<String> orderByColumns, String pinotQuery, List<String> sqlQueries,
+ int h2NumRows, long pinotNumRecordsSelected) throws IOException,
SQLException {
+
+ int pinotNumRows = connectionResultSet.getRowCount();
+ // No record selected in H2
+ if (h2NumRows== 0) {
+ if (pinotNumRows != 0) {
+ String failureMessage = "No record selected in H2 but number of
records selected in Pinot: " + pinotNumRows;
+ failure(pinotQuery, sqlQueries, failureMessage);
+ return;
+ }
+
+ if (pinotNumRecordsSelected != 0) {
+ String failureMessage =
+ "No selection result returned in Pinot but number of records
selected: " + pinotNumRecordsSelected;
+ failure(pinotQuery, sqlQueries, failureMessage);
+ return;
+ }
+
+ // Skip further comparison
+ return;
+ }
+
+ PinotQuery compiledQuery =
CalciteSqlParser.compileToPinotQuery(pinotQuery);
+ boolean isLimitSet = compiledQuery.isSetLimit();
+ int limit = compiledQuery.getLimit();
+
+ // Only compare exhausted results
+ if (h2NumRows < MAX_NUM_ROWS_TO_COMPARE) {
+
+ for (int rowIndex = 0; rowIndex < pinotNumRows; rowIndex++) {
+ // Build actual value String.
+ StringBuilder actualValueBuilder = new StringBuilder();
+ StringBuilder actualOrderByValueBuilder = new StringBuilder();
+ for (int columnIndex = 0; columnIndex <
connectionResultSet.getColumnCount(); columnIndex++) {
+ // Convert column name to all uppercase to make it compatible with H2
+ String columnName =
connectionResultSet.getColumnName(columnIndex).toUpperCase();
+ String columnResult = connectionResultSet.getString(rowIndex,
columnIndex);
+
+ String columnDataType =
connectionResultSet.getColumnDataType(columnIndex);
+ columnResult = removeTrailingZeroForNumber(columnResult,
columnDataType);
+ // TODO: Find a better way to identify multi-value column
+ if (columnResult.charAt(0) == '[') {
+ // Multi-value column
+ JsonNode columnValues = JsonUtils.stringToJsonNode(columnResult);
+ List<String> multiValue = new ArrayList<>();
+ int length = columnValues.size();
+ for (int elementIndex = 0; elementIndex < length; elementIndex++) {
+ multiValue.add(columnValues.get(elementIndex).asText());
+ }
+ for (int elementIndex = length; elementIndex <
MAX_NUM_ELEMENTS_IN_MULTI_VALUE_TO_COMPARE; elementIndex++) {
+ multiValue.add("null");
+ }
+ Collections.sort(multiValue);
+ actualValueBuilder.append(multiValue.toString()).append(' ');
+ if (orderByColumns.contains(columnName)) {
+ actualOrderByValueBuilder.append(columnResult).append(' ');
+ }
+ } else {
+ // Single-value column
+ actualValueBuilder.append(columnResult).append(' ');
+ if (orderByColumns.contains(columnName)) {
+ actualOrderByValueBuilder.append(columnResult).append(' ');
+ }
+ }
+ }
+
+ String actualValue = actualValueBuilder.toString();
+ String actualOrderByValue = actualOrderByValueBuilder.toString();
+ // Check actual value in expected values set, skip comparison if query
response is truncated by limit
+ if ((!isLimitSet || limit > h2NumRows) &&
!expectedValues.contains(actualValue)) {
+ String failureMessage = "Selection result returned in Pinot but not
in H2: " + actualValue + ", " + expectedValues;
+ failure(pinotQuery, sqlQueries, failureMessage);
+ return;
+ }
+ if (!orderByColumns.isEmpty()) {
+ // Check actual group value is the same as expected group value in
the same order.
+ if (!expectedOrderByValues.get(rowIndex).equals(actualOrderByValue))
{
+ String failureMessage = String.format(
+ "Selection Order by result at row index: %d in Pinot: [ %s ]
is different than result in H2: [ %s ].",
+ rowIndex, actualOrderByValue,
expectedOrderByValues.get(rowIndex));
+ failure(pinotQuery, sqlQueries, failureMessage);
+ return;
+ }
+ }
+ }
+ }
+ }
+
+ private static String removeTrailingZeroForNumber(String value, String type)
{
+ // remove trailing zero after decimal point to compare decimal numbers
+ if (type == null || type.toUpperCase().equals("FLOAT") ||
type.toUpperCase().equals("DOUBLE") || type.toUpperCase().equals("BIGINT")) {
+ // remove trailing zero after decimal point to be consistent with h2 data
+ if (value.endsWith(".0")) {
Review comment:
So, this won't work for some cases.
You will need to double check `DataTableReducer` implementations and check
how we format double/float to string.
For instance, `SelectionDataTableReducer` uses
`SelectionOperatorUtils.renderSelectionResultsWithoutOrdering()` and this is
probably causing `x.0` for your case.
On the other hand, if you check `AggregationDataTableReducer`, we use
`AggregationFunctionUtils.formatValue()`, which will convert the decimals to
`x.00000`
So, I think that the better approach is to use the regex like the following:
```
Find the position where
1. start with '.'
2. any number of {0}
3. end with '0'
```
----------------------------------------------------------------
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
