mcvsubbu commented on a change in pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#discussion_r583133785
##########
File path:
pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +73,87 @@ public void setExpectedResultsFileName(String
expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against
results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n",
_queryFileName, _expectedResultsFileName);
+ try {
+ return verifyQueries();
+ } catch (Exception e) {
+ LOGGER.error("FAILED to verify queries in {}: {}", _queryFileName, e);
+ return false;
+ }
+ }
+
+ boolean verifyQueries()
+ throws Exception {
+ boolean testPassed = false;
+
+ try (BufferedReader queryReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_queryFileName),
StandardCharsets.UTF_8));
+ BufferedReader expectedResultReader = new BufferedReader(
+ new InputStreamReader(new
FileInputStream(_expectedResultsFileName), StandardCharsets.UTF_8))) {
+
+ int passed = 0;
+ int total = 0;
+ int queryLineNum = 0;
+ String query;
+
+ while ((query = queryReader.readLine()) != null) {
+ queryLineNum++;
+ if (shouldIgnore(query)) {
+ continue;
+ }
+
+ JsonNode expectedJson = null;
+ try {
+ String expectedResultLine = expectedResultReader.readLine();
+ while (shouldIgnore(expectedResultLine)) {
+ expectedResultLine = expectedResultReader.readLine();
+ }
+ expectedJson = JsonUtils.stringToJsonNode(expectedResultLine);
+ } catch (Exception e) {
+ LOGGER.error("Comparison FAILED: Line: {} Exception caught while
getting expected response for query: '{}'",
+ queryLineNum, query, e);
+ }
+
+ JsonNode actualJson = null;
+ if (expectedJson != null) {
+ try {
+ actualJson = QueryProcessor.postSqlQuery(query);
+ } catch (Exception e) {
+ LOGGER.error("Comparison FAILED: Line: {} Exception caught while
running query: '{}'", queryLineNum, query,
+ e);
+ }
+ }
+
+ if (expectedJson != null && actualJson != null) {
+ try {
+ boolean comparisonResult =
SqlResultComparator.areEqual(actualJson, expectedJson, query);
Review comment:
```suggestion
boolean passed = SqlResultComparator.areEqual(actualJson,
expectedJson, query);
```
##########
File path:
pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +73,87 @@ public void setExpectedResultsFileName(String
expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against
results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n",
_queryFileName, _expectedResultsFileName);
+ try {
+ return verifyQueries();
+ } catch (Exception e) {
+ LOGGER.error("FAILED to verify queries in {}: {}", _queryFileName, e);
+ return false;
+ }
+ }
+
+ boolean verifyQueries()
+ throws Exception {
+ boolean testPassed = false;
+
+ try (BufferedReader queryReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_queryFileName),
StandardCharsets.UTF_8));
+ BufferedReader expectedResultReader = new BufferedReader(
+ new InputStreamReader(new
FileInputStream(_expectedResultsFileName), StandardCharsets.UTF_8))) {
+
+ int passed = 0;
+ int total = 0;
Review comment:
```suggestion
int totalQueryCount = 0;
```
##########
File path:
pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/SegmentOp.java
##########
@@ -67,8 +68,7 @@
private static final int DEFAULT_SLEEP_INTERVAL_MS = 200;
public enum Op {
- UPLOAD,
- DELETE
+ UPLOAD, DELETE
Review comment:
Keep them in separate lines please
##########
File path:
pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +73,87 @@ public void setExpectedResultsFileName(String
expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against
results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n",
_queryFileName, _expectedResultsFileName);
+ try {
+ return verifyQueries();
+ } catch (Exception e) {
+ LOGGER.error("FAILED to verify queries in {}: {}", _queryFileName, e);
+ return false;
+ }
+ }
+
+ boolean verifyQueries()
+ throws Exception {
+ boolean testPassed = false;
+
+ try (BufferedReader queryReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_queryFileName),
StandardCharsets.UTF_8));
+ BufferedReader expectedResultReader = new BufferedReader(
+ new InputStreamReader(new
FileInputStream(_expectedResultsFileName), StandardCharsets.UTF_8))) {
+
+ int passed = 0;
+ int total = 0;
+ int queryLineNum = 0;
+ String query;
+
+ while ((query = queryReader.readLine()) != null) {
+ queryLineNum++;
+ if (shouldIgnore(query)) {
+ continue;
+ }
+
+ JsonNode expectedJson = null;
+ try {
+ String expectedResultLine = expectedResultReader.readLine();
+ while (shouldIgnore(expectedResultLine)) {
+ expectedResultLine = expectedResultReader.readLine();
+ }
+ expectedJson = JsonUtils.stringToJsonNode(expectedResultLine);
+ } catch (Exception e) {
+ LOGGER.error("Comparison FAILED: Line: {} Exception caught while
getting expected response for query: '{}'",
+ queryLineNum, query, e);
+ }
+
+ JsonNode actualJson = null;
+ if (expectedJson != null) {
+ try {
+ actualJson = QueryProcessor.postSqlQuery(query);
+ } catch (Exception e) {
+ LOGGER.error("Comparison FAILED: Line: {} Exception caught while
running query: '{}'", queryLineNum, query,
+ e);
+ }
+ }
+
+ if (expectedJson != null && actualJson != null) {
+ try {
+ boolean comparisonResult =
SqlResultComparator.areEqual(actualJson, expectedJson, query);
+ if (comparisonResult) {
+ passed++;
+ LOGGER.info("Comparison PASSED: Line: {} actual Time: {} ms
expected Time: {} ms Docs Scanned: {}",
Review comment:
We should not be logging expected time, etc. We should not even have it
in metadata. We can add some outliers for time later. For now, remove it from
the results file and also from this log.
Also, we don't need an INFO log for every passing query. Just add a single
debug log with query, expected result and actual result
##########
File path:
pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +73,87 @@ public void setExpectedResultsFileName(String
expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against
results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n",
_queryFileName, _expectedResultsFileName);
+ try {
+ return verifyQueries();
+ } catch (Exception e) {
+ LOGGER.error("FAILED to verify queries in {}: {}", _queryFileName, e);
+ return false;
+ }
+ }
+
+ boolean verifyQueries()
+ throws Exception {
+ boolean testPassed = false;
+
+ try (BufferedReader queryReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_queryFileName),
StandardCharsets.UTF_8));
+ BufferedReader expectedResultReader = new BufferedReader(
+ new InputStreamReader(new
FileInputStream(_expectedResultsFileName), StandardCharsets.UTF_8))) {
+
+ int passed = 0;
Review comment:
```suggestion
int SucceededQueryCount = 0;
```
##########
File path:
pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/SegmentOp.java
##########
@@ -241,6 +242,26 @@ private boolean verifySegmentInState(String state)
return true;
}
+ private boolean verifyRoutingTableUpdated()
Review comment:
You should really be getting the number of rows before adding the
segment, and then the number of rows after adding the segment and make sure
that it has increased by the number of rows in the segment. If you do this,
then you can remove the other check of segment state model.online.
You can mark this as TODO and take it on in a separate PR
##########
File path:
pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +73,87 @@ public void setExpectedResultsFileName(String
expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against
results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n",
_queryFileName, _expectedResultsFileName);
+ try {
+ return verifyQueries();
+ } catch (Exception e) {
+ LOGGER.error("FAILED to verify queries in {}: {}", _queryFileName, e);
+ return false;
+ }
+ }
+
+ boolean verifyQueries()
+ throws Exception {
+ boolean testPassed = false;
+
+ try (BufferedReader queryReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_queryFileName),
StandardCharsets.UTF_8));
+ BufferedReader expectedResultReader = new BufferedReader(
+ new InputStreamReader(new
FileInputStream(_expectedResultsFileName), StandardCharsets.UTF_8))) {
+
+ int passed = 0;
+ int total = 0;
+ int queryLineNum = 0;
+ String query;
+
+ while ((query = queryReader.readLine()) != null) {
+ queryLineNum++;
+ if (shouldIgnore(query)) {
+ continue;
+ }
+
+ JsonNode expectedJson = null;
+ try {
+ String expectedResultLine = expectedResultReader.readLine();
+ while (shouldIgnore(expectedResultLine)) {
+ expectedResultLine = expectedResultReader.readLine();
+ }
+ expectedJson = JsonUtils.stringToJsonNode(expectedResultLine);
+ } catch (Exception e) {
+ LOGGER.error("Comparison FAILED: Line: {} Exception caught while
getting expected response for query: '{}'",
+ queryLineNum, query, e);
+ }
+
+ JsonNode actualJson = null;
+ if (expectedJson != null) {
+ try {
+ actualJson = QueryProcessor.postSqlQuery(query);
+ } catch (Exception e) {
+ LOGGER.error("Comparison FAILED: Line: {} Exception caught while
running query: '{}'", queryLineNum, query,
+ e);
+ }
+ }
+
+ if (expectedJson != null && actualJson != null) {
+ try {
+ boolean comparisonResult =
SqlResultComparator.areEqual(actualJson, expectedJson, query);
+ if (comparisonResult) {
+ passed++;
+ LOGGER.info("Comparison PASSED: Line: {} actual Time: {} ms
expected Time: {} ms Docs Scanned: {}",
+ queryLineNum, actualJson.get(TIME_USED_MS_KEY),
expectedJson.get(TIME_USED_MS_KEY),
+ actualJson.get(NUM_DOCS_SCANNED_KEY));
+ LOGGER.debug("actual Response: {}", actualJson);
Review comment:
Please combine these two logs into one log.
##########
File path:
pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +73,87 @@ public void setExpectedResultsFileName(String
expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against
results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n",
_queryFileName, _expectedResultsFileName);
+ try {
+ return verifyQueries();
+ } catch (Exception e) {
+ LOGGER.error("FAILED to verify queries in {}: {}", _queryFileName, e);
+ return false;
+ }
+ }
+
+ boolean verifyQueries()
+ throws Exception {
+ boolean testPassed = false;
+
+ try (BufferedReader queryReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_queryFileName),
StandardCharsets.UTF_8));
+ BufferedReader expectedResultReader = new BufferedReader(
+ new InputStreamReader(new
FileInputStream(_expectedResultsFileName), StandardCharsets.UTF_8))) {
+
+ int passed = 0;
+ int total = 0;
+ int queryLineNum = 0;
+ String query;
+
+ while ((query = queryReader.readLine()) != null) {
+ queryLineNum++;
+ if (shouldIgnore(query)) {
+ continue;
+ }
+
+ JsonNode expectedJson = null;
+ try {
+ String expectedResultLine = expectedResultReader.readLine();
+ while (shouldIgnore(expectedResultLine)) {
+ expectedResultLine = expectedResultReader.readLine();
+ }
+ expectedJson = JsonUtils.stringToJsonNode(expectedResultLine);
+ } catch (Exception e) {
+ LOGGER.error("Comparison FAILED: Line: {} Exception caught while
getting expected response for query: '{}'",
+ queryLineNum, query, e);
+ }
+
+ JsonNode actualJson = null;
+ if (expectedJson != null) {
+ try {
+ actualJson = QueryProcessor.postSqlQuery(query);
+ } catch (Exception e) {
+ LOGGER.error("Comparison FAILED: Line: {} Exception caught while
running query: '{}'", queryLineNum, query,
+ e);
+ }
+ }
+
+ if (expectedJson != null && actualJson != null) {
+ try {
+ boolean comparisonResult =
SqlResultComparator.areEqual(actualJson, expectedJson, query);
+ if (comparisonResult) {
+ passed++;
+ LOGGER.info("Comparison PASSED: Line: {} actual Time: {} ms
expected Time: {} ms Docs Scanned: {}",
+ queryLineNum, actualJson.get(TIME_USED_MS_KEY),
expectedJson.get(TIME_USED_MS_KEY),
+ actualJson.get(NUM_DOCS_SCANNED_KEY));
+ LOGGER.debug("actual Response: {}", actualJson);
+ LOGGER.debug("expected Response: {}", expectedJson);
+ } else {
+ LOGGER.error("Comparison FAILED: Line: {} query: {}",
queryLineNum, query);
Review comment:
Single log.error will suffice.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
