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 2021/05/25 06:56:24 UTC
[GitHub] [incubator-pinot] mqliang opened a new pull request #6969: Fix partial comparison for realtime table and allow keep cluster on failure
mqliang opened a new pull request #6969:
URL: https://github.com/apache/incubator-pinot/pull/6969
## Description
<!-- Add a description of your PR here.
A good description should include pointers to an issue or design document, etc.
-->
This PR:
* There is a superset/partial comparison query for realtime table, update the query result
* Pinot server do some early termination optimization (process in parallel and early return if get enough documents to fulfill the LIMIT and OFFSET requirement) for some queries. In this case, divide the result into several groups and for the last group, only compare the ordered by column values.
* Update the shell script to allow user pass an optional flag "--keep-cluster-on-failure" to allow keep cluster for debugging on test failure.
cc @siddharthteotia @GSharayu @mcvsubbu @jackjlli
## Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
* [ ] Yes (Please label as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR fix a zero-downtime upgrade introduced earlier?
* [ ] Yes (Please label this as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR otherwise need attention when creating release notes? Things to consider:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
* [ ] Yes (Please label this PR as **<code>release-notes</code>** and complete the section on Release Notes)
## Release Notes
<!-- If you have tagged this as either backward-incompat or release-notes,
you MUST add text here that you would like to see appear in release notes of the
next release. -->
<!-- If you have a series of commits adding or enabling a feature, then
add this section only in final commit that marks the feature completed.
Refer to earlier release notes to see examples of text.
-->
## Documentation
<!-- If you have introduced a new feature or configuration, please add it to the documentation as well.
See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document
-->
--
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-commenter edited a comment on pull request #6969: Fix partial comparison for realtime table and allow keep cluster on failure
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #6969:
URL: https://github.com/apache/incubator-pinot/pull/6969#issuecomment-847666543
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6969?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#6969](https://codecov.io/gh/apache/incubator-pinot/pull/6969?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (65c02a8) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/9adbe558322037126ca4cb281e1a2a7b3f22e3fd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9adbe55) will **increase** coverage by `0.05%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6969?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #6969 +/- ##
============================================
+ Coverage 73.29% 73.35% +0.05%
Complexity 12 12
============================================
Files 1432 1441 +9
Lines 71035 71450 +415
Branches 10292 10354 +62
============================================
+ Hits 52065 52410 +345
- Misses 15485 15517 +32
- Partials 3485 3523 +38
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `42.01% <ø> (-0.89%)` | :arrow_down: |
| unittests | `65.52% <ø> (+0.20%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6969?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...data/manager/realtime/SegmentCommitterFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvU2VnbWVudENvbW1pdHRlckZhY3RvcnkuamF2YQ==) | `64.70% <0.00%> (-35.30%)` | :arrow_down: |
| [...t/server/api/resources/SegmentMetadataFetcher.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvYXBpL3Jlc291cmNlcy9TZWdtZW50TWV0YWRhdGFGZXRjaGVyLmphdmE=) | `45.00% <0.00%> (-24.05%)` | :arrow_down: |
| [...pache/pinot/spi/env/CommonsConfigurationUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZW52L0NvbW1vbnNDb25maWd1cmF0aW9uVXRpbHMuamF2YQ==) | `60.00% <0.00%> (-20.00%)` | :arrow_down: |
| [.../helix/core/minion/MinionInstancesCleanupTask.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9NaW5pb25JbnN0YW5jZXNDbGVhbnVwVGFzay5qYXZh) | `57.14% <0.00%> (-14.29%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/utils/EqualityUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvRXF1YWxpdHlVdGlscy5qYXZh) | `62.79% <0.00%> (-13.08%)` | :arrow_down: |
| [...altime/ServerSegmentCompletionProtocolHandler.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VydmVyL3JlYWx0aW1lL1NlcnZlclNlZ21lbnRDb21wbGV0aW9uUHJvdG9jb2xIYW5kbGVyLmphdmE=) | `48.54% <0.00%> (-8.74%)` | :arrow_down: |
| [...er/api/resources/LLCSegmentCompletionHandlers.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL0xMQ1NlZ21lbnRDb21wbGV0aW9uSGFuZGxlcnMuamF2YQ==) | `51.16% <0.00%> (-8.38%)` | :arrow_down: |
| [...e/data/manager/realtime/SplitSegmentCommitter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvU3BsaXRTZWdtZW50Q29tbWl0dGVyLmphdmE=) | `53.84% <0.00%> (-7.70%)` | :arrow_down: |
| [...impl/dictionary/DoubleOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRG91YmxlT25IZWFwTXV0YWJsZURpY3Rpb25hcnkuamF2YQ==) | `46.98% <0.00%> (-7.23%)` | :arrow_down: |
| [...operator/filter/RangeIndexBasedFilterOperator.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvUmFuZ2VJbmRleEJhc2VkRmlsdGVyT3BlcmF0b3IuamF2YQ==) | `40.42% <0.00%> (-4.26%)` | :arrow_down: |
| ... and [105 more](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6969?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6969?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [9adbe55...65c02a8](https://codecov.io/gh/apache/incubator-pinot/pull/6969?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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] mqliang commented on a change in pull request #6969: Fix partial comparison for realtime table and allow keep cluster on failure
Posted by GitBox <gi...@apache.org>.
mqliang commented on a change in pull request #6969:
URL: https://github.com/apache/incubator-pinot/pull/6969#discussion_r641259035
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/SqlResultComparator.java
##########
@@ -113,9 +126,147 @@ public static boolean areEqual(JsonNode actual, JsonNode expected, String query)
if (expected.has(FIELD_IS_SUPERSET) && expected.get(FIELD_IS_SUPERSET).asBoolean(false)) {
return areElementsSubset(actualElementsSerialized, expectedElementsSerialized);
} else {
- return areLengthsEqual(actual, expected) && areElementsEqual(actualElementsSerialized, expectedElementsSerialized,
- query) && areMetadataEqual(actual, expected);
+ if (!areLengthsEqual(actual, expected)) {
+ return false;
+ }
+ /*
+ * Pinot server do some early termination optimization (process in parallel and early return if get enough
+ * documents to fulfill the LIMIT and OFFSET requirement) for queries:
+ * - selection without order by
+ * - selection with order by (by sorting the segments on min-max value)
+ * - DISTINCT queries.
+ * numDocsScanned is no-deterministic for those queries so numDocsScanned comparison should be skipped.
+ *
Review comment:
done
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/SqlResultComparator.java
##########
@@ -113,9 +126,147 @@ public static boolean areEqual(JsonNode actual, JsonNode expected, String query)
if (expected.has(FIELD_IS_SUPERSET) && expected.get(FIELD_IS_SUPERSET).asBoolean(false)) {
return areElementsSubset(actualElementsSerialized, expectedElementsSerialized);
} else {
- return areLengthsEqual(actual, expected) && areElementsEqual(actualElementsSerialized, expectedElementsSerialized,
- query) && areMetadataEqual(actual, expected);
+ if (!areLengthsEqual(actual, expected)) {
+ return false;
+ }
+ /*
+ * Pinot server do some early termination optimization (process in parallel and early return if get enough
+ * documents to fulfill the LIMIT and OFFSET requirement) for queries:
+ * - selection without order by
+ * - selection with order by (by sorting the segments on min-max value)
+ * - DISTINCT queries.
+ * numDocsScanned is no-deterministic for those queries so numDocsScanned comparison should be skipped.
+ *
+ * NOTE: DISTINCT queries are modeled as non-selection queries during query processing, but all DISTINCT
+ * queries are selection queries during Calcite parsing (DISTINCT queries are selection queries with
+ * selectNode.getModifierNode(SqlSelectKeyword.DISTINCT) != null).
+ */
+ if (!isSelectionQuery(query) && !areNumDocsScannedEqual(actual, expected)) {
+ return false;
+ }
+ return isOrderByQuery(query) ? areOrderByQueryElementsEqual(actualRows, expectedRows, actualElementsSerialized,
+ expectedElementsSerialized, query)
+ : areNonOrderByQueryElementsEqual(actualElementsSerialized, expectedElementsSerialized);
+ }
+ }
+
+ private static boolean areOrderByQueryElementsEqual(ArrayNode actualElements, ArrayNode expectedElements,
+ List<String> actualElementsSerialized, List<String> expectedElementsSerialized, String query) {
+ // Happy path, the results match exactly.
+ if (actualElementsSerialized.equals(expectedElementsSerialized)) {
+ LOGGER.debug("The results of the ordered query match exactly!");
+ return true;
+ }
+
+ /*
+ * Unhappy path, it possible that the returned results:
+ * - are not ordered by all columns.
+ * - to be a subset of total qualified results.
+ * In this case, we divide the results into groups (based on the ordered by column values), then compare the actual
+ * results and expected results group by group:
+ * - ordered by column values should be the same.
+ * - other column values (columns not in the order-by list) should be in the same set.
+ * - for the last group, since it's possible that the returned results to be a subset of total qualified results,
+ * skipping the value comparison for other columns.
+ *
+ * Let's say we have a table:
+ * column_name: A, B, C
+ * row 0 value: 1, 2, 3
+ * row 1 value: 2, 2, 4
+ * row 2 value: 3, 7, 5
+ * row 3 value: 3, 2, 3
+ * row 4 value: 4, 2, 5
+ * row 5 value: 4, 7, 6
+ *
+ * There are 4 possible result for query `SELECT * from table ordered by A LIMIT 5`:
+ * 1) [row 0, row 1, row 2, row 3, row 4]
+ * 2) [row 0, row 1, row 3, row 2, row 4]
+ * 3) [row 0, row 1, row 2, row 3, row 5]
+ * 4) [row 0, row 1, row 3, row 2, row 5]
+ *
+ * So we will divide the result into 4 groups (based on value of A):
+ * group 1: [row 0]
+ * group 2: [row 1]
+ * group 3: [row 2, row 3], or [row 3, row 2]
+ * group 4: [row 4] or [row 5]
+ *
+ * During comparison:
+ * - for group 1, 2, 3: value of A should be the same, value of (B, C) should be in the same set.
+ * - for group 4: only verify the value of A should be the same.
+ */
+
+ List<Integer> orderByColumnIndexs = getOrderByColumnIndexs(query);
+ LinkedHashMap<String, List<String>> actualOrderByColumnValuesToOtherColumnValuesMap = new LinkedHashMap<>();
+ LinkedHashMap<String, List<String>> expectedOrderByColumnValuesToOtherColumnValuesMap = new LinkedHashMap<>();
+ String lastGroupOrderByColumnValues = "";
+ for (int i = 0; i < actualElements.size(); i++) {
+ String actualOrderByColumnValues = "";
+ String expectedOrderByColumnValues = "";
+ String actualOtherColumnValues = "";
+ String expectOtherColumnValues = "";
+ ArrayNode actualValue = (ArrayNode) actualElements.get(i).get(FIELD_VALUE);
+ ArrayNode expectedValue = (ArrayNode) expectedElements.get(i).get(FIELD_VALUE);
+
+ for (int j = 0; j < actualValue.size(); j++) {
+ if (orderByColumnIndexs.contains(j)) {
+ actualOrderByColumnValues += ", " + actualValue.get(j).toString();
+ expectedOrderByColumnValues += ", " + expectedValue.get(j).toString();
+ } else {
+ actualOtherColumnValues += ", " + actualValue.get(j).toString();
+ expectOtherColumnValues += ", " + expectedValue.get(j).toString();
+ }
+ }
+ lastGroupOrderByColumnValues = actualOrderByColumnValues;
+
+ actualOrderByColumnValuesToOtherColumnValuesMap.
+ computeIfAbsent(actualOrderByColumnValues, k -> new LinkedList<>()).
+ add(actualOtherColumnValues);
+ expectedOrderByColumnValuesToOtherColumnValuesMap.
+ computeIfAbsent(expectedOrderByColumnValues, k -> new LinkedList<>()).
+ add(expectOtherColumnValues);
+ }
+
+ if (!actualOrderByColumnValuesToOtherColumnValuesMap.keySet().
+ equals(expectedOrderByColumnValuesToOtherColumnValuesMap.keySet())) {
+ LOGGER.error("The results of the ordered query has different groups, actual: {}, expected: {}",
+ actualOrderByColumnValuesToOtherColumnValuesMap.keySet(),
+ expectedOrderByColumnValuesToOtherColumnValuesMap.keySet());
+ return false;
+ }
+
+ for (Map.Entry<String, List<String>> entry : actualOrderByColumnValuesToOtherColumnValuesMap.entrySet()) {
+ String orderByColumnValues = entry.getKey();
+ // For the last group, skip the value comparison for other columns.
+ if (orderByColumnValues.equals(lastGroupOrderByColumnValues)) {
+ continue;
+ }
+ List<String> actualOtherColumnValues = entry.getValue();
+ List<String> expectedOtherColumnValues =
+ expectedOrderByColumnValuesToOtherColumnValuesMap.get(orderByColumnValues);
+ Collections.sort(actualOtherColumnValues);
+ Collections.sort(expectedOtherColumnValues);
+ if (!actualOtherColumnValues.equals(expectedOtherColumnValues)) {
+ LOGGER.error(
+ "The results of the ordered query has different non-order-by column values for group: {}, actual: {}, expected: {}",
+ orderByColumnValues, actualOtherColumnValues, expectedOtherColumnValues);
+ return false;
+ }
+ }
+
+ return true;
+ }
+
+ private static boolean areNonOrderByQueryElementsEqual(List<String> actualElementsSerialized,
+ List<String> expectedElementsSerialized) {
+ // sort elements
+ actualElementsSerialized.sort(null);
+ expectedElementsSerialized.sort(null);
+ if (!actualElementsSerialized.equals(expectedElementsSerialized)) {
Review comment:
done
##########
File path: compatibility-verifier/README.md
##########
@@ -32,7 +32,13 @@ dir and output the path, which can be used in step 2.
### Step 2: run compatibility regression test against the two targets build in step1
```shell
-./compatibility-verifier/compCheck.sh [workingDir]
+./compCheck.sh -h
+Usage: -w <workingDir> -t <testSuiteDir> -k true/false
Review comment:
done
##########
File path: compatibility-verifier/compCheck.sh
##########
@@ -41,20 +41,54 @@
RM="/bin/rm"
logCount=1
+#Declare the number of mandatory args
+margs=2
# get usage of the script
function usage() {
command=$1
- echo "Usage: $command workingDir testSuiteDir"
- exit 1
+ echo "Usage: $command -w <workingDir> -t <testSuiteDir> -k true/false"
+}
+
+function help() {
+ usage
+ echo -e "MANDATORY:"
+ echo -e " -w, --working-dir Working directory where olderCommit and newCommit target files reside."
+ echo -e " -t, --test-suite-dir Test suite directory\n"
+ echo -e "OPTIONAL:"
+ echo -e " -k, --keep-cluster-on-failure Whether keep cluster on test failure (default: false)"
Review comment:
done
--
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] mcvsubbu commented on pull request #6969: Fix partial comparison for realtime table and allow keep cluster on failure
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #6969:
URL: https://github.com/apache/incubator-pinot/pull/6969#issuecomment-851606720
@siddharthteotia merging this for now, if you have more concerns regarding your comment, or have a more elegant way of solving that problem, we can fix it in a later PR.
--
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-commenter commented on pull request #6969: Fix partial comparison for realtime table and allow keep cluster on failure
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #6969:
URL: https://github.com/apache/incubator-pinot/pull/6969#issuecomment-847666543
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6969?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#6969](https://codecov.io/gh/apache/incubator-pinot/pull/6969?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (010d3e8) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/9adbe558322037126ca4cb281e1a2a7b3f22e3fd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9adbe55) will **decrease** coverage by `7.98%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6969?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #6969 +/- ##
============================================
- Coverage 73.29% 65.31% -7.99%
Complexity 12 12
============================================
Files 1432 1435 +3
Lines 71035 71169 +134
Branches 10292 10310 +18
============================================
- Hits 52065 46482 -5583
- Misses 15485 21321 +5836
+ Partials 3485 3366 -119
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `?` | |
| unittests | `65.31% <ø> (-0.02%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6969?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...a/org/apache/pinot/minion/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/minion/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUXVlcnlDbGllbnQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...t/core/startree/plan/StarTreeDocIdSetPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlRG9jSWRTZXRQbGFuTm9kZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../core/startree/plan/StarTreeTransformPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlVHJhbnNmb3JtUGxhbk5vZGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...core/startree/plan/StarTreeProjectionPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlUHJvamVjdGlvblBsYW5Ob2RlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...t/minion/executor/MinionTaskZkMetadataManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhlY3V0b3IvTWluaW9uVGFza1prTWV0YWRhdGFNYW5hZ2VyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...startree/executor/StarTreeAggregationExecutor.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9leGVjdXRvci9TdGFyVHJlZUFnZ3JlZ2F0aW9uRXhlY3V0b3IuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [387 more](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6969?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6969?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [9adbe55...010d3e8](https://codecov.io/gh/apache/incubator-pinot/pull/6969?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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] siddharthteotia commented on a change in pull request #6969: Fix partial comparison for realtime table and allow keep cluster on failure
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6969:
URL: https://github.com/apache/incubator-pinot/pull/6969#discussion_r639071638
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/SqlResultComparator.java
##########
@@ -113,9 +126,147 @@ public static boolean areEqual(JsonNode actual, JsonNode expected, String query)
if (expected.has(FIELD_IS_SUPERSET) && expected.get(FIELD_IS_SUPERSET).asBoolean(false)) {
return areElementsSubset(actualElementsSerialized, expectedElementsSerialized);
} else {
- return areLengthsEqual(actual, expected) && areElementsEqual(actualElementsSerialized, expectedElementsSerialized,
- query) && areMetadataEqual(actual, expected);
+ if (!areLengthsEqual(actual, expected)) {
+ return false;
+ }
+ /*
+ * Pinot server do some early termination optimization (process in parallel and early return if get enough
+ * documents to fulfill the LIMIT and OFFSET requirement) for queries:
+ * - selection without order by
+ * - selection with order by (by sorting the segments on min-max value)
+ * - DISTINCT queries.
+ * numDocsScanned is no-deterministic for those queries so numDocsScanned comparison should be skipped.
+ *
+ * NOTE: DISTINCT queries are modeled as non-selection queries during query processing, but all DISTINCT
+ * queries are selection queries during Calcite parsing (DISTINCT queries are selection queries with
+ * selectNode.getModifierNode(SqlSelectKeyword.DISTINCT) != null).
+ */
+ if (!isSelectionQuery(query) && !areNumDocsScannedEqual(actual, expected)) {
+ return false;
+ }
+ return isOrderByQuery(query) ? areOrderByQueryElementsEqual(actualRows, expectedRows, actualElementsSerialized,
+ expectedElementsSerialized, query)
+ : areNonOrderByQueryElementsEqual(actualElementsSerialized, expectedElementsSerialized);
+ }
+ }
+
+ private static boolean areOrderByQueryElementsEqual(ArrayNode actualElements, ArrayNode expectedElements,
+ List<String> actualElementsSerialized, List<String> expectedElementsSerialized, String query) {
+ // Happy path, the results match exactly.
+ if (actualElementsSerialized.equals(expectedElementsSerialized)) {
+ LOGGER.debug("The results of the ordered query match exactly!");
+ return true;
+ }
+
+ /*
+ * Unhappy path, it possible that the returned results:
+ * - are not ordered by all columns.
+ * - to be a subset of total qualified results.
+ * In this case, we divide the results into groups (based on the ordered by column values), then compare the actual
+ * results and expected results group by group:
+ * - ordered by column values should be the same.
+ * - other column values (columns not in the order-by list) should be in the same set.
+ * - for the last group, since it's possible that the returned results to be a subset of total qualified results,
+ * skipping the value comparison for other columns.
+ *
+ * Let's say we have a table:
+ * column_name: A, B, C
+ * row 0 value: 1, 2, 3
+ * row 1 value: 2, 2, 4
+ * row 2 value: 3, 7, 5
+ * row 3 value: 3, 2, 3
+ * row 4 value: 4, 2, 5
+ * row 5 value: 4, 7, 6
+ *
+ * There are 4 possible result for query `SELECT * from table ordered by A LIMIT 5`:
+ * 1) [row 0, row 1, row 2, row 3, row 4]
+ * 2) [row 0, row 1, row 3, row 2, row 4]
+ * 3) [row 0, row 1, row 2, row 3, row 5]
+ * 4) [row 0, row 1, row 3, row 2, row 5]
+ *
+ * So we will divide the result into 4 groups (based on value of A):
+ * group 1: [row 0]
+ * group 2: [row 1]
+ * group 3: [row 2, row 3], or [row 3, row 2]
+ * group 4: [row 4] or [row 5]
+ *
+ * During comparison:
+ * - for group 1, 2, 3: value of A should be the same, value of (B, C) should be in the same set.
+ * - for group 4: only verify the value of A should be the same.
+ */
+
+ List<Integer> orderByColumnIndexs = getOrderByColumnIndexs(query);
+ LinkedHashMap<String, List<String>> actualOrderByColumnValuesToOtherColumnValuesMap = new LinkedHashMap<>();
+ LinkedHashMap<String, List<String>> expectedOrderByColumnValuesToOtherColumnValuesMap = new LinkedHashMap<>();
+ String lastGroupOrderByColumnValues = "";
+ for (int i = 0; i < actualElements.size(); i++) {
+ String actualOrderByColumnValues = "";
+ String expectedOrderByColumnValues = "";
+ String actualOtherColumnValues = "";
+ String expectOtherColumnValues = "";
+ ArrayNode actualValue = (ArrayNode) actualElements.get(i).get(FIELD_VALUE);
+ ArrayNode expectedValue = (ArrayNode) expectedElements.get(i).get(FIELD_VALUE);
+
+ for (int j = 0; j < actualValue.size(); j++) {
+ if (orderByColumnIndexs.contains(j)) {
+ actualOrderByColumnValues += ", " + actualValue.get(j).toString();
+ expectedOrderByColumnValues += ", " + expectedValue.get(j).toString();
+ } else {
+ actualOtherColumnValues += ", " + actualValue.get(j).toString();
+ expectOtherColumnValues += ", " + expectedValue.get(j).toString();
+ }
+ }
+ lastGroupOrderByColumnValues = actualOrderByColumnValues;
+
+ actualOrderByColumnValuesToOtherColumnValuesMap.
+ computeIfAbsent(actualOrderByColumnValues, k -> new LinkedList<>()).
+ add(actualOtherColumnValues);
+ expectedOrderByColumnValuesToOtherColumnValuesMap.
+ computeIfAbsent(expectedOrderByColumnValues, k -> new LinkedList<>()).
+ add(expectOtherColumnValues);
+ }
+
+ if (!actualOrderByColumnValuesToOtherColumnValuesMap.keySet().
+ equals(expectedOrderByColumnValuesToOtherColumnValuesMap.keySet())) {
+ LOGGER.error("The results of the ordered query has different groups, actual: {}, expected: {}",
+ actualOrderByColumnValuesToOtherColumnValuesMap.keySet(),
+ expectedOrderByColumnValuesToOtherColumnValuesMap.keySet());
+ return false;
+ }
+
+ for (Map.Entry<String, List<String>> entry : actualOrderByColumnValuesToOtherColumnValuesMap.entrySet()) {
+ String orderByColumnValues = entry.getKey();
+ // For the last group, skip the value comparison for other columns.
+ if (orderByColumnValues.equals(lastGroupOrderByColumnValues)) {
+ continue;
+ }
+ List<String> actualOtherColumnValues = entry.getValue();
+ List<String> expectedOtherColumnValues =
+ expectedOrderByColumnValuesToOtherColumnValuesMap.get(orderByColumnValues);
+ Collections.sort(actualOtherColumnValues);
+ Collections.sort(expectedOtherColumnValues);
+ if (!actualOtherColumnValues.equals(expectedOtherColumnValues)) {
+ LOGGER.error(
+ "The results of the ordered query has different non-order-by column values for group: {}, actual: {}, expected: {}",
+ orderByColumnValues, actualOtherColumnValues, expectedOtherColumnValues);
+ return false;
+ }
+ }
+
+ return true;
+ }
+
+ private static boolean areNonOrderByQueryElementsEqual(List<String> actualElementsSerialized,
+ List<String> expectedElementsSerialized) {
+ // sort elements
+ actualElementsSerialized.sort(null);
+ expectedElementsSerialized.sort(null);
+ if (!actualElementsSerialized.equals(expectedElementsSerialized)) {
Review comment:
Pinot doesn't guarantee deterministic results (rows) for multiple executions of queries without ORDER BY. How do we know that a query without ORDER BY will return same results when run twice ?
--
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] mcvsubbu merged pull request #6969: Fix partial comparison for realtime table and allow keep cluster on failure
Posted by GitBox <gi...@apache.org>.
mcvsubbu merged pull request #6969:
URL: https://github.com/apache/incubator-pinot/pull/6969
--
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] mcvsubbu commented on a change in pull request #6969: Fix partial comparison for realtime table and allow keep cluster on failure
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6969:
URL: https://github.com/apache/incubator-pinot/pull/6969#discussion_r639012635
##########
File path: compatibility-verifier/README.md
##########
@@ -32,7 +32,13 @@ dir and output the path, which can be used in step 2.
### Step 2: run compatibility regression test against the two targets build in step1
```shell
-./compatibility-verifier/compCheck.sh [workingDir]
+./compCheck.sh -h
+Usage: -w <workingDir> -t <testSuiteDir> -k true/false
Review comment:
```suggestion
Usage: -w <workingDir> -t <testSuiteDir> [-k]
```
##########
File path: compatibility-verifier/README.md
##########
@@ -32,7 +32,13 @@ dir and output the path, which can be used in step 2.
### Step 2: run compatibility regression test against the two targets build in step1
```shell
-./compatibility-verifier/compCheck.sh [workingDir]
+./compCheck.sh -h
+Usage: -w <workingDir> -t <testSuiteDir> -k true/false
+MANDATORY:
+ -w, --working-dir Working directory where olderCommit and newCommit target files reside.
+ -t, --test-suite-dir Test suite directory
+
+OPTIONAL:
+ -k, --keep-cluster-on-failure Whether keep cluster on test failure (default: false)
Review comment:
```suggestion
-k, --keep-cluster-on-failure Keep cluster on test failure
```
##########
File path: compatibility-verifier/compCheck.sh
##########
@@ -41,20 +41,54 @@
RM="/bin/rm"
logCount=1
+#Declare the number of mandatory args
+margs=2
# get usage of the script
function usage() {
command=$1
- echo "Usage: $command workingDir testSuiteDir"
- exit 1
+ echo "Usage: $command -w <workingDir> -t <testSuiteDir> -k true/false"
+}
+
+function help() {
+ usage
+ echo -e "MANDATORY:"
+ echo -e " -w, --working-dir Working directory where olderCommit and newCommit target files reside."
+ echo -e " -t, --test-suite-dir Test suite directory\n"
+ echo -e "OPTIONAL:"
+ echo -e " -k, --keep-cluster-on-failure Whether keep cluster on test failure (default: false)"
Review comment:
```suggestion
echo -e " -k, --keep-cluster-on-failure Keep cluster on test failure"
```
##########
File path: compatibility-verifier/compCheck.sh
##########
@@ -41,20 +41,54 @@
RM="/bin/rm"
logCount=1
+#Declare the number of mandatory args
+margs=2
# get usage of the script
function usage() {
command=$1
- echo "Usage: $command workingDir testSuiteDir"
- exit 1
+ echo "Usage: $command -w <workingDir> -t <testSuiteDir> -k true/false"
+}
+
+function help() {
+ usage
+ echo -e "MANDATORY:"
+ echo -e " -w, --working-dir Working directory where olderCommit and newCommit target files reside."
+ echo -e " -t, --test-suite-dir Test suite directory\n"
+ echo -e "OPTIONAL:"
+ echo -e " -k, --keep-cluster-on-failure Whether keep cluster on test failure (default: false)"
+ echo -e " -h, --help Prints this help\n"
+}
+
+# Ensures that the number of passed args are at least equals
+# to the declared number of mandatory args.
+# It also handles the special case of the -h or --help arg.
+function margs_precheck() {
+ if [ $2 ] && [ $1 -lt $margs ]; then
+ if [ $2 == "--help" ] || [ $2 == "-h" ]; then
+ help
+ exit
+ else
+ usage compCheck
+ exit 1 # error
+ fi
+ fi
+}
+
+# Ensures that all the mandatory args are not empty
+function margs_check() {
+ if [ $# -lt $margs ]; then
Review comment:
will be good if oldTargetDir and newTargetDir presence is also checked in workingDir
--
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] mqliang commented on a change in pull request #6969: Fix partial comparison for realtime table and allow keep cluster on failure
Posted by GitBox <gi...@apache.org>.
mqliang commented on a change in pull request #6969:
URL: https://github.com/apache/incubator-pinot/pull/6969#discussion_r641260050
##########
File path: compatibility-verifier/compCheck.sh
##########
@@ -41,20 +41,54 @@
RM="/bin/rm"
logCount=1
+#Declare the number of mandatory args
+margs=2
# get usage of the script
function usage() {
command=$1
- echo "Usage: $command workingDir testSuiteDir"
- exit 1
+ echo "Usage: $command -w <workingDir> -t <testSuiteDir> -k true/false"
+}
+
+function help() {
+ usage
+ echo -e "MANDATORY:"
+ echo -e " -w, --working-dir Working directory where olderCommit and newCommit target files reside."
+ echo -e " -t, --test-suite-dir Test suite directory\n"
+ echo -e "OPTIONAL:"
+ echo -e " -k, --keep-cluster-on-failure Whether keep cluster on test failure (default: false)"
+ echo -e " -h, --help Prints this help\n"
+}
+
+# Ensures that the number of passed args are at least equals
+# to the declared number of mandatory args.
+# It also handles the special case of the -h or --help arg.
+function margs_precheck() {
+ if [ $2 ] && [ $1 -lt $margs ]; then
+ if [ $2 == "--help" ] || [ $2 == "-h" ]; then
+ help
+ exit
+ else
+ usage compCheck
+ exit 1 # error
+ fi
+ fi
+}
+
+# Ensures that all the mandatory args are not empty
+function margs_check() {
+ if [ $# -lt $margs ]; then
Review comment:
not add the check yet, since if the oldTargetDir and newTargetDir not exist, the test will fail anyway.
--
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] siddharthteotia commented on a change in pull request #6969: Fix partial comparison for realtime table and allow keep cluster on failure
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6969:
URL: https://github.com/apache/incubator-pinot/pull/6969#discussion_r639070818
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/SqlResultComparator.java
##########
@@ -113,9 +126,147 @@ public static boolean areEqual(JsonNode actual, JsonNode expected, String query)
if (expected.has(FIELD_IS_SUPERSET) && expected.get(FIELD_IS_SUPERSET).asBoolean(false)) {
return areElementsSubset(actualElementsSerialized, expectedElementsSerialized);
} else {
- return areLengthsEqual(actual, expected) && areElementsEqual(actualElementsSerialized, expectedElementsSerialized,
- query) && areMetadataEqual(actual, expected);
+ if (!areLengthsEqual(actual, expected)) {
+ return false;
+ }
+ /*
+ * Pinot server do some early termination optimization (process in parallel and early return if get enough
+ * documents to fulfill the LIMIT and OFFSET requirement) for queries:
+ * - selection without order by
+ * - selection with order by (by sorting the segments on min-max value)
+ * - DISTINCT queries.
+ * numDocsScanned is no-deterministic for those queries so numDocsScanned comparison should be skipped.
+ *
Review comment:
(nit) typo non-deterministic
--
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-commenter edited a comment on pull request #6969: Fix partial comparison for realtime table and allow keep cluster on failure
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #6969:
URL: https://github.com/apache/incubator-pinot/pull/6969#issuecomment-847666543
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6969?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#6969](https://codecov.io/gh/apache/incubator-pinot/pull/6969?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (65c02a8) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/9adbe558322037126ca4cb281e1a2a7b3f22e3fd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9adbe55) will **decrease** coverage by `7.76%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6969?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #6969 +/- ##
============================================
- Coverage 73.29% 65.52% -7.77%
Complexity 12 12
============================================
Files 1432 1441 +9
Lines 71035 71450 +415
Branches 10292 10354 +62
============================================
- Hits 52065 46819 -5246
- Misses 15485 21242 +5757
+ Partials 3485 3389 -96
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `?` | |
| unittests | `65.52% <ø> (+0.20%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6969?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...a/org/apache/pinot/minion/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/minion/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUXVlcnlDbGllbnQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...t/core/startree/plan/StarTreeDocIdSetPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlRG9jSWRTZXRQbGFuTm9kZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../core/startree/plan/StarTreeTransformPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlVHJhbnNmb3JtUGxhbk5vZGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...core/startree/plan/StarTreeProjectionPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlUHJvamVjdGlvblBsYW5Ob2RlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...t/minion/executor/MinionTaskZkMetadataManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhlY3V0b3IvTWluaW9uVGFza1prTWV0YWRhdGFNYW5hZ2VyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...startree/executor/StarTreeAggregationExecutor.java](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9leGVjdXRvci9TdGFyVHJlZUFnZ3JlZ2F0aW9uRXhlY3V0b3IuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [403 more](https://codecov.io/gh/apache/incubator-pinot/pull/6969/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6969?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6969?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [9adbe55...65c02a8](https://codecov.io/gh/apache/incubator-pinot/pull/6969?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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] mcvsubbu commented on a change in pull request #6969: Fix partial comparison for realtime table and allow keep cluster on failure
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6969:
URL: https://github.com/apache/incubator-pinot/pull/6969#discussion_r640043943
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/SqlResultComparator.java
##########
@@ -113,9 +126,147 @@ public static boolean areEqual(JsonNode actual, JsonNode expected, String query)
if (expected.has(FIELD_IS_SUPERSET) && expected.get(FIELD_IS_SUPERSET).asBoolean(false)) {
return areElementsSubset(actualElementsSerialized, expectedElementsSerialized);
} else {
- return areLengthsEqual(actual, expected) && areElementsEqual(actualElementsSerialized, expectedElementsSerialized,
- query) && areMetadataEqual(actual, expected);
+ if (!areLengthsEqual(actual, expected)) {
+ return false;
+ }
+ /*
+ * Pinot server do some early termination optimization (process in parallel and early return if get enough
+ * documents to fulfill the LIMIT and OFFSET requirement) for queries:
+ * - selection without order by
+ * - selection with order by (by sorting the segments on min-max value)
+ * - DISTINCT queries.
+ * numDocsScanned is no-deterministic for those queries so numDocsScanned comparison should be skipped.
+ *
+ * NOTE: DISTINCT queries are modeled as non-selection queries during query processing, but all DISTINCT
+ * queries are selection queries during Calcite parsing (DISTINCT queries are selection queries with
+ * selectNode.getModifierNode(SqlSelectKeyword.DISTINCT) != null).
+ */
+ if (!isSelectionQuery(query) && !areNumDocsScannedEqual(actual, expected)) {
+ return false;
+ }
+ return isOrderByQuery(query) ? areOrderByQueryElementsEqual(actualRows, expectedRows, actualElementsSerialized,
+ expectedElementsSerialized, query)
+ : areNonOrderByQueryElementsEqual(actualElementsSerialized, expectedElementsSerialized);
+ }
+ }
+
+ private static boolean areOrderByQueryElementsEqual(ArrayNode actualElements, ArrayNode expectedElements,
+ List<String> actualElementsSerialized, List<String> expectedElementsSerialized, String query) {
+ // Happy path, the results match exactly.
+ if (actualElementsSerialized.equals(expectedElementsSerialized)) {
+ LOGGER.debug("The results of the ordered query match exactly!");
+ return true;
+ }
+
+ /*
+ * Unhappy path, it possible that the returned results:
+ * - are not ordered by all columns.
+ * - to be a subset of total qualified results.
+ * In this case, we divide the results into groups (based on the ordered by column values), then compare the actual
+ * results and expected results group by group:
+ * - ordered by column values should be the same.
+ * - other column values (columns not in the order-by list) should be in the same set.
+ * - for the last group, since it's possible that the returned results to be a subset of total qualified results,
+ * skipping the value comparison for other columns.
+ *
+ * Let's say we have a table:
+ * column_name: A, B, C
+ * row 0 value: 1, 2, 3
+ * row 1 value: 2, 2, 4
+ * row 2 value: 3, 7, 5
+ * row 3 value: 3, 2, 3
+ * row 4 value: 4, 2, 5
+ * row 5 value: 4, 7, 6
+ *
+ * There are 4 possible result for query `SELECT * from table ordered by A LIMIT 5`:
+ * 1) [row 0, row 1, row 2, row 3, row 4]
+ * 2) [row 0, row 1, row 3, row 2, row 4]
+ * 3) [row 0, row 1, row 2, row 3, row 5]
+ * 4) [row 0, row 1, row 3, row 2, row 5]
+ *
+ * So we will divide the result into 4 groups (based on value of A):
+ * group 1: [row 0]
+ * group 2: [row 1]
+ * group 3: [row 2, row 3], or [row 3, row 2]
+ * group 4: [row 4] or [row 5]
+ *
+ * During comparison:
+ * - for group 1, 2, 3: value of A should be the same, value of (B, C) should be in the same set.
+ * - for group 4: only verify the value of A should be the same.
+ */
+
+ List<Integer> orderByColumnIndexs = getOrderByColumnIndexs(query);
+ LinkedHashMap<String, List<String>> actualOrderByColumnValuesToOtherColumnValuesMap = new LinkedHashMap<>();
+ LinkedHashMap<String, List<String>> expectedOrderByColumnValuesToOtherColumnValuesMap = new LinkedHashMap<>();
+ String lastGroupOrderByColumnValues = "";
+ for (int i = 0; i < actualElements.size(); i++) {
+ String actualOrderByColumnValues = "";
+ String expectedOrderByColumnValues = "";
+ String actualOtherColumnValues = "";
+ String expectOtherColumnValues = "";
+ ArrayNode actualValue = (ArrayNode) actualElements.get(i).get(FIELD_VALUE);
+ ArrayNode expectedValue = (ArrayNode) expectedElements.get(i).get(FIELD_VALUE);
+
+ for (int j = 0; j < actualValue.size(); j++) {
+ if (orderByColumnIndexs.contains(j)) {
+ actualOrderByColumnValues += ", " + actualValue.get(j).toString();
+ expectedOrderByColumnValues += ", " + expectedValue.get(j).toString();
+ } else {
+ actualOtherColumnValues += ", " + actualValue.get(j).toString();
+ expectOtherColumnValues += ", " + expectedValue.get(j).toString();
+ }
+ }
+ lastGroupOrderByColumnValues = actualOrderByColumnValues;
+
+ actualOrderByColumnValuesToOtherColumnValuesMap.
+ computeIfAbsent(actualOrderByColumnValues, k -> new LinkedList<>()).
+ add(actualOtherColumnValues);
+ expectedOrderByColumnValuesToOtherColumnValuesMap.
+ computeIfAbsent(expectedOrderByColumnValues, k -> new LinkedList<>()).
+ add(expectOtherColumnValues);
+ }
+
+ if (!actualOrderByColumnValuesToOtherColumnValuesMap.keySet().
+ equals(expectedOrderByColumnValuesToOtherColumnValuesMap.keySet())) {
+ LOGGER.error("The results of the ordered query has different groups, actual: {}, expected: {}",
+ actualOrderByColumnValuesToOtherColumnValuesMap.keySet(),
+ expectedOrderByColumnValuesToOtherColumnValuesMap.keySet());
+ return false;
+ }
+
+ for (Map.Entry<String, List<String>> entry : actualOrderByColumnValuesToOtherColumnValuesMap.entrySet()) {
+ String orderByColumnValues = entry.getKey();
+ // For the last group, skip the value comparison for other columns.
+ if (orderByColumnValues.equals(lastGroupOrderByColumnValues)) {
+ continue;
+ }
+ List<String> actualOtherColumnValues = entry.getValue();
+ List<String> expectedOtherColumnValues =
+ expectedOrderByColumnValuesToOtherColumnValuesMap.get(orderByColumnValues);
+ Collections.sort(actualOtherColumnValues);
+ Collections.sort(expectedOtherColumnValues);
+ if (!actualOtherColumnValues.equals(expectedOtherColumnValues)) {
+ LOGGER.error(
+ "The results of the ordered query has different non-order-by column values for group: {}, actual: {}, expected: {}",
+ orderByColumnValues, actualOtherColumnValues, expectedOtherColumnValues);
+ return false;
+ }
+ }
+
+ return true;
+ }
+
+ private static boolean areNonOrderByQueryElementsEqual(List<String> actualElementsSerialized,
+ List<String> expectedElementsSerialized) {
+ // sort elements
+ actualElementsSerialized.sort(null);
+ expectedElementsSerialized.sort(null);
+ if (!actualElementsSerialized.equals(expectedElementsSerialized)) {
Review comment:
For non-orderby queries, the result file should specify the isSuperset flag. If that is done, we will never hit this case.
@mqliang it may be a good idea to add that as a comment here, so that if people see failures, they can update the result file.
--
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