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/11/03 17:32:08 UTC
[GitHub] [incubator-pinot] jtao15 opened a new pull request #6224: Improve comparison coverage for selection SQL queries in ClusterInteg…
jtao15 opened a new pull request #6224:
URL: https://github.com/apache/incubator-pinot/pull/6224
Improve comparison logic for sql selection queries similar as pql.
- Compare non order by single columns
- Compare multi-value columns
- Compare queries which order by columns are not in selection clause
----------------------------------------------------------------
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] snleee commented on a change in pull request #6224: Improve comparison coverage for selection SQL queries in ClusterInteg…
Posted by GitBox <gi...@apache.org>.
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:
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] jtao15 commented on a change in pull request #6224: Improve comparison coverage for selection SQL queries in ClusterInteg…
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #6224:
URL: https://github.com/apache/incubator-pinot/pull/6224#discussion_r521663461
##########
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:
I think the problem here is whether we want to check the returned order of selection queries which order-by columns are not in selection clause. We skipped comparison for this case for previous sql comparison, and current pql logic will not verify the order but only compare values. So without the order-by clause append, the sql comparison will be consistent with current pql logic, and all test cases should pass.
----------------------------------------------------------------
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 edited a comment on pull request #6224: Improve comparison coverage for selection SQL queries in ClusterInteg…
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6224:
URL: https://github.com/apache/incubator-pinot/pull/6224#issuecomment-721873395
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6224?src=pr&el=h1) Report
> Merging [#6224](https://codecov.io/gh/apache/incubator-pinot/pull/6224?src=pr&el=desc) (e784bd2) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **increase** coverage by `6.79%`.
> The diff coverage is `68.40%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6224?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6224 +/- ##
==========================================
+ Coverage 66.44% 73.24% +6.79%
==========================================
Files 1075 1243 +168
Lines 54773 58949 +4176
Branches 8168 8739 +571
==========================================
+ Hits 36396 43175 +6779
+ Misses 15700 12942 -2758
- Partials 2677 2832 +155
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `46.28% <52.01%> (?)` | |
| unittests | `64.11% <45.12%> (?)` | |
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/6224?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `44.44% <0.00%> (-4.40%)` | :arrow_down: |
| [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/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/6224/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/6224/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/6224/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRGF0ZVRpbWVQYXR0ZXJuSGFuZGxlci5qYXZh) | `83.33% <ø> (ø)` | |
| [...ot/common/function/FunctionDefinitionRegistry.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRnVuY3Rpb25EZWZpbml0aW9uUmVnaXN0cnkuamF2YQ==) | `88.88% <ø> (+44.44%)` | :arrow_up: |
| [...org/apache/pinot/common/function/FunctionInfo.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRnVuY3Rpb25JbmZvLmphdmE=) | `100.00% <ø> (ø)` | |
| ... and [1020 more](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6224?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/6224?src=pr&el=footer). Last update [5a53fbe...e784bd2](https://codecov.io/gh/apache/incubator-pinot/pull/6224?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 a change in pull request #6224: Improve comparison coverage for selection SQL queries in ClusterInteg…
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6224:
URL: https://github.com/apache/incubator-pinot/pull/6224#discussion_r520187785
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/ClusterIntegrationTestUtils.java
##########
@@ -463,21 +466,20 @@ private static Object generateRandomValue(Schema.Type fieldType) {
* </ul>
*
* @param pinotQuery Pinot query
- * @param queryFormat Pinot query format
* @param brokerUrl Pinot broker URL
* @param pinotConnection Pinot connection
* @param sqlQueries H2 SQL queries
* @param h2Connection H2 connection
* @throws Exception
*/
- public static void testQuery(String pinotQuery, String queryFormat, String brokerUrl,
+ public static void testQuery(String pinotQuery, String brokerUrl,
org.apache.pinot.client.Connection pinotConnection, @Nullable List<String> sqlQueries,
@Nullable Connection h2Connection)
throws Exception {
// Use broker response for metadata check, connection response for value check
- PinotQueryRequest pinotBrokerQueryRequest = new PinotQueryRequest(queryFormat, pinotQuery);
+ PinotQueryRequest pinotBrokerQueryRequest = new PinotQueryRequest("pql", pinotQuery);
Review comment:
Use `CommonConstants.Broker.Request.PQL` here?
##########
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:
If the values of lastName is exhausted, we can still verify the values, right?
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/ClusterIntegrationTestUtils.java
##########
@@ -463,21 +466,20 @@ private static Object generateRandomValue(Schema.Type fieldType) {
* </ul>
*
* @param pinotQuery Pinot query
- * @param queryFormat Pinot query format
* @param brokerUrl Pinot broker URL
* @param pinotConnection Pinot connection
* @param sqlQueries H2 SQL queries
* @param h2Connection H2 connection
* @throws Exception
*/
- public static void testQuery(String pinotQuery, String queryFormat, String brokerUrl,
+ public static void testQuery(String pinotQuery, String brokerUrl,
Review comment:
+1 on that
----------------------------------------------------------------
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] jtao15 commented on a change in pull request #6224: Improve comparison coverage for selection SQL queries in ClusterInteg…
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #6224:
URL: https://github.com/apache/incubator-pinot/pull/6224#discussion_r521667786
##########
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:
5 here is the length of h2 muti-value column suffix. A typical h2 multi-value column name will be 'NAME__MV0'. Maybe it's better to change it as a constant instead of number.
----------------------------------------------------------------
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] snleee commented on a change in pull request #6224: Improve comparison coverage for selection SQL queries in ClusterInteg…
Posted by GitBox <gi...@apache.org>.
snleee commented on a change in pull request #6224:
URL: https://github.com/apache/incubator-pinot/pull/6224#discussion_r518302566
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/ClusterIntegrationTestUtils.java
##########
@@ -463,21 +466,20 @@ private static Object generateRandomValue(Schema.Type fieldType) {
* </ul>
*
* @param pinotQuery Pinot query
- * @param queryFormat Pinot query format
* @param brokerUrl Pinot broker URL
* @param pinotConnection Pinot connection
* @param sqlQueries H2 SQL queries
* @param h2Connection H2 connection
* @throws Exception
*/
- public static void testQuery(String pinotQuery, String queryFormat, String brokerUrl,
+ public static void testQuery(String pinotQuery, String brokerUrl,
Review comment:
Can we rename this to `testPqlQuery()`?
----------------------------------------------------------------
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 #6224: Improve comparison coverage for selection SQL queries in ClusterInteg…
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6224:
URL: https://github.com/apache/incubator-pinot/pull/6224#discussion_r517605955
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/ClusterIntegrationTestUtils.java
##########
@@ -666,149 +668,14 @@ public static void testQuery(String pinotQuery, String queryFormat, String broke
}
Set<String> expectedValues = new HashSet<>();
List<String> expectedOrderByValues = new ArrayList<>();
- Map<String, String> reusableExpectedValueMap = new HashMap<>();
- Map<String, List<String>> reusableMultiValuesMap = new HashMap<>();
- List<String> reusableColumnOrder = new ArrayList<>();
- int h2NumRows;
- for (h2NumRows = 0; h2ResultSet.next() && h2NumRows < MAX_NUM_ROWS_TO_COMPARE; h2NumRows++) {
- reusableExpectedValueMap.clear();
- reusableMultiValuesMap.clear();
- reusableColumnOrder.clear();
-
- int numColumns = h2MetaData.getColumnCount();
- for (int columnIndex = 1; columnIndex <= numColumns; columnIndex++) {
- 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
- 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(column).append(':').append(reusableExpectedValueMap.get(column)).append(' ');
- if (orderByColumns.contains(column)) {
- expectedOrderByValue.append(column).append(':').append(reusableExpectedValueMap.get(column)).append(' ');
- }
- }
- expectedValues.add(expectedValue.toString());
- expectedOrderByValues.add(expectedOrderByValue.toString());
- }
+ int h2NumRows = getH2ExpectedValues(expectedValues, expectedOrderByValues, h2ResultSet, h2MetaData, orderByColumns);
Review comment:
If the result comparisons for PQL and SQL are done in two separate methods, should we still need to pass the queryFormat into `testQuery()` method?
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/ClusterIntegrationTestUtils.java
##########
@@ -975,6 +830,199 @@ private static boolean isSelectionQuery(BrokerRequest brokerRequest) {
return false;
}
+ private static Set<String> convertToUpperCase(Set<String> columns) {
+ Set<String> upperCaseColumns = new HashSet<>();
+ for (String column: columns) {
+ upperCaseColumns.add(column.toUpperCase());
+ }
+ return upperCaseColumns;
+ }
+
+ private static int getH2ExpectedValues(Set<String> expectedValues, List<String> expectedOrderByValues,
+ ResultSet h2ResultSet, ResultSetMetaData h2MetaData, Set<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++) {
Review comment:
Can you add a comment why the index starts at 1 instead of 0?
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/ClusterIntegrationTestUtils.java
##########
@@ -975,6 +830,199 @@ private static boolean isSelectionQuery(BrokerRequest brokerRequest) {
return false;
}
+ private static Set<String> convertToUpperCase(Set<String> columns) {
+ Set<String> upperCaseColumns = new HashSet<>();
+ for (String column: columns) {
+ upperCaseColumns.add(column.toUpperCase());
+ }
+ return upperCaseColumns;
+ }
+
+ private static int getH2ExpectedValues(Set<String> expectedValues, List<String> expectedOrderByValues,
+ ResultSet h2ResultSet, ResultSetMetaData h2MetaData, Set<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++) {
+ 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, Set<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")) {
+ return value.substring(0, value.length() - 2);
+ }
+ }
+ return value;
+ }
+
+ private static List<String> appendColumnsToSelectionRequests(Set<String> columns, List<String> requests) {
Review comment:
Can you add some comments on why we do it this way in front of this method?
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/ClusterIntegrationTestUtils.java
##########
@@ -975,6 +830,199 @@ private static boolean isSelectionQuery(BrokerRequest brokerRequest) {
return false;
}
+ private static Set<String> convertToUpperCase(Set<String> columns) {
+ Set<String> upperCaseColumns = new HashSet<>();
Review comment:
Is it possible to reuse the original set here?
----------------------------------------------------------------
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 edited a comment on pull request #6224: Improve comparison coverage for selection SQL queries in ClusterInteg…
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6224:
URL: https://github.com/apache/incubator-pinot/pull/6224#issuecomment-721873395
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6224?src=pr&el=h1) Report
> Merging [#6224](https://codecov.io/gh/apache/incubator-pinot/pull/6224?src=pr&el=desc) (d6dcc8d) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **increase** coverage by `6.87%`.
> The diff coverage is `68.27%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6224?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6224 +/- ##
==========================================
+ Coverage 66.44% 73.31% +6.87%
==========================================
Files 1075 1243 +168
Lines 54773 58949 +4176
Branches 8168 8739 +571
==========================================
+ Hits 36396 43221 +6825
+ Misses 15700 12891 -2809
- Partials 2677 2837 +160
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `46.50% <51.88%> (?)` | |
| unittests | `64.10% <45.12%> (?)` | |
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/6224?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `44.44% <0.00%> (-4.40%)` | :arrow_down: |
| [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/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/6224/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/6224/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/6224/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRGF0ZVRpbWVQYXR0ZXJuSGFuZGxlci5qYXZh) | `83.33% <ø> (ø)` | |
| [...ot/common/function/FunctionDefinitionRegistry.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRnVuY3Rpb25EZWZpbml0aW9uUmVnaXN0cnkuamF2YQ==) | `88.88% <ø> (+44.44%)` | :arrow_up: |
| [...org/apache/pinot/common/function/FunctionInfo.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRnVuY3Rpb25JbmZvLmphdmE=) | `100.00% <ø> (ø)` | |
| ... and [1020 more](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6224?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/6224?src=pr&el=footer). Last update [750af31...d6dcc8d](https://codecov.io/gh/apache/incubator-pinot/pull/6224?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] codecov-io commented on pull request #6224: Improve comparison coverage for selection SQL queries in ClusterInteg…
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6224:
URL: https://github.com/apache/incubator-pinot/pull/6224#issuecomment-721873395
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6224?src=pr&el=h1) Report
> Merging [#6224](https://codecov.io/gh/apache/incubator-pinot/pull/6224?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) will **decrease** coverage by `2.33%`.
> The diff coverage is `43.43%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6224?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6224 +/- ##
==========================================
- Coverage 66.44% 64.11% -2.34%
==========================================
Files 1075 1240 +165
Lines 54773 58938 +4165
Branches 8168 8737 +569
==========================================
+ Hits 36396 37787 +1391
- Misses 15700 18398 +2698
- Partials 2677 2753 +76
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `64.11% <43.43%> (?)` | |
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/6224?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <0.00%> (-79.32%)` | :arrow_down: |
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `33.96% <0.00%> (-32.71%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `66.66% <0.00%> (+9.52%)` | :arrow_up: |
| [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `35.55% <0.00%> (-13.29%)` | :arrow_down: |
| [...inot/client/JsonAsyncHttpPinotClientTransport.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0pzb25Bc3luY0h0dHBQaW5vdENsaWVudFRyYW5zcG9ydC5qYXZh) | `10.90% <0.00%> (-51.10%)` | :arrow_down: |
| [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `73.80% <ø> (+0.63%)` | :arrow_up: |
| [.../apache/pinot/common/exception/QueryException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/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/6224/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vQWdncmVnYXRpb25GdW5jdGlvblR5cGUuamF2YQ==) | `98.27% <ø> (-1.73%)` | :arrow_down: |
| ... and [1078 more](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6224?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/6224?src=pr&el=footer). Last update [bee125e...068aac9](https://codecov.io/gh/apache/incubator-pinot/pull/6224?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 a change in pull request #6224: Improve comparison coverage for selection SQL queries in ClusterInteg…
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6224:
URL: https://github.com/apache/incubator-pinot/pull/6224#discussion_r522652419
##########
File path: pinot-integration-tests/src/test/resources/On_Time_On_Time_Performance_2014_100k_subset.test_queries_500.sql
##########
@@ -555,3 +555,9 @@
{"sql":"SELECT WheelsOff, TotalAddGTime, \"Month\", OriginState FROM mytable ORDER BY OriginState LIMIT 24","hsqls":["SELECT WheelsOff, TotalAddGTime, Month, OriginState FROM mytable ORDER BY OriginState LIMIT 24"]}
{"sql":"SELECT WheelsOn, OriginState FROM mytable WHERE DepartureDelayGroups > -2 ORDER BY OriginState LIMIT 20"}
{"sql":"SELECT \"Year\", DepTime FROM mytable WHERE CRSArrTime BETWEEN 1237 AND 1534 AND Cancelled BETWEEN 0 AND 1 ORDER BY DepTime LIMIT 3","hsqls":["SELECT Year, DepTime FROM mytable WHERE CRSArrTime BETWEEN 1237 AND 1534 AND Cancelled BETWEEN 0 AND 1 ORDER BY DepTime LIMIT 3"]}
+{"sql":"SELECT DivDistance, ActualElapsedTime FROM mytable LIMIT 29"}
Review comment:
You can add `# Selection` above this line, and update the line 516 to `#Selection & Order by` to differentiate two groups.
----------------------------------------------------------------
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 edited a comment on pull request #6224: Improve comparison coverage for selection SQL queries in ClusterInteg…
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6224:
URL: https://github.com/apache/incubator-pinot/pull/6224#issuecomment-721873395
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6224?src=pr&el=h1) Report
> Merging [#6224](https://codecov.io/gh/apache/incubator-pinot/pull/6224?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) will **decrease** coverage by `2.35%`.
> The diff coverage is `43.43%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6224?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6224 +/- ##
==========================================
- Coverage 66.44% 64.09% -2.36%
==========================================
Files 1075 1240 +165
Lines 54773 58938 +4165
Branches 8168 8737 +569
==========================================
+ Hits 36396 37774 +1378
- Misses 15700 18411 +2711
- Partials 2677 2753 +76
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `64.09% <43.43%> (?)` | |
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/6224?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <0.00%> (-79.32%)` | :arrow_down: |
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `33.96% <0.00%> (-32.71%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `66.66% <0.00%> (+9.52%)` | :arrow_up: |
| [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `35.55% <0.00%> (-13.29%)` | :arrow_down: |
| [...inot/client/JsonAsyncHttpPinotClientTransport.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0pzb25Bc3luY0h0dHBQaW5vdENsaWVudFRyYW5zcG9ydC5qYXZh) | `10.90% <0.00%> (-51.10%)` | :arrow_down: |
| [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `73.80% <ø> (+0.63%)` | :arrow_up: |
| [.../apache/pinot/common/exception/QueryException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/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/6224/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vQWdncmVnYXRpb25GdW5jdGlvblR5cGUuamF2YQ==) | `98.27% <ø> (-1.73%)` | :arrow_down: |
| ... and [1077 more](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6224?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/6224?src=pr&el=footer). Last update [bee125e...aab1cfa](https://codecov.io/gh/apache/incubator-pinot/pull/6224?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] jtao15 commented on a change in pull request #6224: Improve comparison coverage for selection SQL queries in ClusterInteg…
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #6224:
URL: https://github.com/apache/incubator-pinot/pull/6224#discussion_r521702422
##########
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:
I'll remove this to be consistent with pql comparison, the values are still being checked.
----------------------------------------------------------------
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] jtao15 commented on a change in pull request #6224: Improve comparison coverage for selection SQL queries in ClusterInteg…
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #6224:
URL: https://github.com/apache/incubator-pinot/pull/6224#discussion_r520773713
##########
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:
Yes, the values are checked. Adding the order-by columns will help to verify the order, the values are still checked by comparing hashed row records.
----------------------------------------------------------------
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] snleee commented on a change in pull request #6224: Improve comparison coverage for selection SQL queries in ClusterInteg…
Posted by GitBox <gi...@apache.org>.
snleee commented on a change in pull request #6224:
URL: https://github.com/apache/incubator-pinot/pull/6224#discussion_r522519514
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/ClusterIntegrationTestUtils.java
##########
@@ -975,6 +814,203 @@ 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 > H2_MULTI_VALUE_SUFFIX_LENGTH && columnName.substring(length - H2_MULTI_VALUE_SUFFIX_LENGTH, length - 1).equals("__MV")) {
+ // Multi-value column
+ String multiValueColumnName = columnName.substring(0, length - H2_MULTI_VALUE_SUFFIX_LENGTH);
+ 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);
+
+ boolean isMultiValueColumn = false;
+ try {
+ isMultiValueColumn = JsonUtils.stringToJsonNode(columnResult).isArray();
+ } catch (IOException e) {
+ }
+
+ if (isMultiValueColumn) {
+ // Multi-value column
+ JsonNode columnValues = JsonUtils.stringToJsonNode(columnResult);
Review comment:
We can parse JsonNode once when computing `isMultiValueColumn` and reuse it. Your code parses JSON 2 times.
##########
File path: pinot-integration-tests/src/test/resources/On_Time_On_Time_Performance_2014_100k_subset.test_queries_500.sql
##########
@@ -555,3 +555,9 @@
{"sql":"SELECT WheelsOff, TotalAddGTime, \"Month\", OriginState FROM mytable ORDER BY OriginState LIMIT 24","hsqls":["SELECT WheelsOff, TotalAddGTime, Month, OriginState FROM mytable ORDER BY OriginState LIMIT 24"]}
{"sql":"SELECT WheelsOn, OriginState FROM mytable WHERE DepartureDelayGroups > -2 ORDER BY OriginState LIMIT 20"}
{"sql":"SELECT \"Year\", DepTime FROM mytable WHERE CRSArrTime BETWEEN 1237 AND 1534 AND Cancelled BETWEEN 0 AND 1 ORDER BY DepTime LIMIT 3","hsqls":["SELECT Year, DepTime FROM mytable WHERE CRSArrTime BETWEEN 1237 AND 1534 AND Cancelled BETWEEN 0 AND 1 ORDER BY DepTime LIMIT 3"]}
+{"sql":"SELECT DivDistance, ActualElapsedTime FROM mytable LIMIT 29"}
+{"sql":"SELECT DayOfWeek FROM mytable WHERE TotalAddGTime IN (128, 148, 4, 34) LIMIT 8"}
+{"sql":"SELECT WheelsOff FROM mytable WHERE DivDistance < 436 LIMIT 12"}
+{"sql":"SELECT DivDistance, DepTime FROM mytable WHERE NASDelay IN (45, 55, 31, 9) LIMIT 26"}
+{"sql":"SELECT DepDelay FROM mytable WHERE DepDelayMinutes BETWEEN 292.0 AND 237.0 AND DestState IN ('DE', 'AZ') LIMIT 18"}
+{"sql":"SELECT DestStateFips FROM mytable LIMIT 27"}
Review comment:
add extra line in the end
----------------------------------------------------------------
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] jtao15 commented on a change in pull request #6224: Improve comparison coverage for selection SQL queries in ClusterInteg…
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #6224:
URL: https://github.com/apache/incubator-pinot/pull/6224#discussion_r517667947
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/ClusterIntegrationTestUtils.java
##########
@@ -666,149 +668,14 @@ public static void testQuery(String pinotQuery, String queryFormat, String broke
}
Set<String> expectedValues = new HashSet<>();
List<String> expectedOrderByValues = new ArrayList<>();
- Map<String, String> reusableExpectedValueMap = new HashMap<>();
- Map<String, List<String>> reusableMultiValuesMap = new HashMap<>();
- List<String> reusableColumnOrder = new ArrayList<>();
- int h2NumRows;
- for (h2NumRows = 0; h2ResultSet.next() && h2NumRows < MAX_NUM_ROWS_TO_COMPARE; h2NumRows++) {
- reusableExpectedValueMap.clear();
- reusableMultiValuesMap.clear();
- reusableColumnOrder.clear();
-
- int numColumns = h2MetaData.getColumnCount();
- for (int columnIndex = 1; columnIndex <= numColumns; columnIndex++) {
- 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
- 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(column).append(':').append(reusableExpectedValueMap.get(column)).append(' ');
- if (orderByColumns.contains(column)) {
- expectedOrderByValue.append(column).append(':').append(reusableExpectedValueMap.get(column)).append(' ');
- }
- }
- expectedValues.add(expectedValue.toString());
- expectedOrderByValues.add(expectedOrderByValue.toString());
- }
+ int h2NumRows = getH2ExpectedValues(expectedValues, expectedOrderByValues, h2ResultSet, h2MetaData, orderByColumns);
Review comment:
You are right, I think we can remove `queryFormat` cause it will always be 'pql' for `testQuery()`
----------------------------------------------------------------
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 edited a comment on pull request #6224: Improve comparison coverage for selection SQL queries in ClusterInteg…
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6224:
URL: https://github.com/apache/incubator-pinot/pull/6224#issuecomment-721873395
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6224?src=pr&el=h1) Report
> Merging [#6224](https://codecov.io/gh/apache/incubator-pinot/pull/6224?src=pr&el=desc) (d6dcc8d) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `2.34%`.
> The diff coverage is `45.12%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6224?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6224 +/- ##
==========================================
- Coverage 66.44% 64.10% -2.35%
==========================================
Files 1075 1243 +168
Lines 54773 58949 +4176
Branches 8168 8739 +571
==========================================
+ Hits 36396 37789 +1393
- Misses 15700 18402 +2702
- Partials 2677 2758 +81
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `64.10% <45.12%> (?)` | |
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/6224?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <0.00%> (-79.32%)` | :arrow_down: |
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `33.96% <0.00%> (-32.71%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `66.66% <0.00%> (+9.52%)` | :arrow_up: |
| [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `35.55% <0.00%> (-13.29%)` | :arrow_down: |
| [...inot/client/JsonAsyncHttpPinotClientTransport.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0pzb25Bc3luY0h0dHBQaW5vdENsaWVudFRyYW5zcG9ydC5qYXZh) | `10.90% <0.00%> (-51.10%)` | :arrow_down: |
| [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `73.80% <ø> (+0.63%)` | :arrow_up: |
| [.../apache/pinot/common/exception/QueryException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/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/6224/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vQWdncmVnYXRpb25GdW5jdGlvblR5cGUuamF2YQ==) | `98.27% <ø> (-1.73%)` | :arrow_down: |
| ... and [1082 more](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6224?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/6224?src=pr&el=footer). Last update [750af31...d6dcc8d](https://codecov.io/gh/apache/incubator-pinot/pull/6224?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] codecov-io edited a comment on pull request #6224: Improve comparison coverage for selection SQL queries in ClusterInteg…
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6224:
URL: https://github.com/apache/incubator-pinot/pull/6224#issuecomment-721873395
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6224?src=pr&el=h1) Report
> Merging [#6224](https://codecov.io/gh/apache/incubator-pinot/pull/6224?src=pr&el=desc) (83a277e) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **increase** coverage by `6.80%`.
> The diff coverage is `68.27%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6224?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6224 +/- ##
==========================================
+ Coverage 66.44% 73.25% +6.80%
==========================================
Files 1075 1243 +168
Lines 54773 58949 +4176
Branches 8168 8739 +571
==========================================
+ Hits 36396 43182 +6786
+ Misses 15700 12931 -2769
- Partials 2677 2836 +159
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `46.34% <51.88%> (?)` | |
| unittests | `64.11% <45.12%> (?)` | |
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/6224?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `44.44% <0.00%> (-4.40%)` | :arrow_down: |
| [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/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/6224/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/6224/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/6224/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRGF0ZVRpbWVQYXR0ZXJuSGFuZGxlci5qYXZh) | `83.33% <ø> (ø)` | |
| [...ot/common/function/FunctionDefinitionRegistry.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRnVuY3Rpb25EZWZpbml0aW9uUmVnaXN0cnkuamF2YQ==) | `88.88% <ø> (+44.44%)` | :arrow_up: |
| [...org/apache/pinot/common/function/FunctionInfo.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRnVuY3Rpb25JbmZvLmphdmE=) | `100.00% <ø> (ø)` | |
| ... and [1017 more](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6224?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/6224?src=pr&el=footer). Last update [5a53fbe...21eacde](https://codecov.io/gh/apache/incubator-pinot/pull/6224?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] codecov-io edited a comment on pull request #6224: Improve comparison coverage for selection SQL queries in ClusterInteg…
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6224:
URL: https://github.com/apache/incubator-pinot/pull/6224#issuecomment-721873395
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6224?src=pr&el=h1) Report
> Merging [#6224](https://codecov.io/gh/apache/incubator-pinot/pull/6224?src=pr&el=desc) (e784bd2) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `2.33%`.
> The diff coverage is `45.12%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6224?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6224 +/- ##
==========================================
- Coverage 66.44% 64.11% -2.34%
==========================================
Files 1075 1243 +168
Lines 54773 58949 +4176
Branches 8168 8739 +571
==========================================
+ Hits 36396 37797 +1401
- Misses 15700 18400 +2700
- Partials 2677 2752 +75
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `64.11% <45.12%> (?)` | |
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/6224?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <0.00%> (-79.32%)` | :arrow_down: |
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `33.96% <0.00%> (-32.71%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `66.66% <0.00%> (+9.52%)` | :arrow_up: |
| [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `35.55% <0.00%> (-13.29%)` | :arrow_down: |
| [...inot/client/JsonAsyncHttpPinotClientTransport.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0pzb25Bc3luY0h0dHBQaW5vdENsaWVudFRyYW5zcG9ydC5qYXZh) | `10.90% <0.00%> (-51.10%)` | :arrow_down: |
| [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `73.80% <ø> (+0.63%)` | :arrow_up: |
| [.../apache/pinot/common/exception/QueryException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/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/6224/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vQWdncmVnYXRpb25GdW5jdGlvblR5cGUuamF2YQ==) | `98.27% <ø> (-1.73%)` | :arrow_down: |
| ... and [1082 more](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6224?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/6224?src=pr&el=footer). Last update [5a53fbe...e784bd2](https://codecov.io/gh/apache/incubator-pinot/pull/6224?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] codecov-io edited a comment on pull request #6224: Improve comparison coverage for selection SQL queries in ClusterInteg…
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6224:
URL: https://github.com/apache/incubator-pinot/pull/6224#issuecomment-721873395
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6224?src=pr&el=h1) Report
> Merging [#6224](https://codecov.io/gh/apache/incubator-pinot/pull/6224?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) will **decrease** coverage by `20.04%`.
> The diff coverage is `51.01%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6224?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6224 +/- ##
===========================================
- Coverage 66.44% 46.40% -20.05%
===========================================
Files 1075 1240 +165
Lines 54773 58938 +4165
Branches 8168 8737 +569
===========================================
- Hits 36396 27351 -9045
- Misses 15700 29348 +13648
+ Partials 2677 2239 -438
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `46.40% <51.01%> (?)` | |
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/6224?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...org/apache/pinot/broker/queryquota/HitCounter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IaXRDb3VudGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...che/pinot/broker/queryquota/MaxHitRateTracker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9NYXhIaXRSYXRlVHJhY2tlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...ache/pinot/broker/queryquota/QueryQuotaEntity.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9RdWVyeVF1b3RhRW50aXR5LmphdmE=) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `22.22% <0.00%> (-26.62%)` | :arrow_down: |
| [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `64.28% <ø> (-8.89%)` | :arrow_down: |
| [.../apache/pinot/common/exception/QueryException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZXhjZXB0aW9uL1F1ZXJ5RXhjZXB0aW9uLmphdmE=) | `90.27% <ø> (+5.55%)` | :arrow_up: |
| ... and [1227 more](https://codecov.io/gh/apache/incubator-pinot/pull/6224/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6224?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/6224?src=pr&el=footer). Last update [bee125e...1fe905b](https://codecov.io/gh/apache/incubator-pinot/pull/6224?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] snleee commented on a change in pull request #6224: Improve comparison coverage for selection SQL queries in ClusterInteg…
Posted by GitBox <gi...@apache.org>.
snleee commented on a change in pull request #6224:
URL: https://github.com/apache/incubator-pinot/pull/6224#discussion_r521683891
##########
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:
let's use the constant
----------------------------------------------------------------
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] snleee merged pull request #6224: Improve comparison coverage for selection SQL queries in ClusterInteg…
Posted by GitBox <gi...@apache.org>.
snleee merged pull request #6224:
URL: https://github.com/apache/incubator-pinot/pull/6224
----------------------------------------------------------------
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