Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12221 )
Change subject: IMPALA-5872: Testcase builder for query planner ...................................................................... Patch Set 2: (3 comments) Minor suggestions, otherwise looks pretty good. Since this code won't affect production use cases, my thought is that we should merge it early so we can all play with and extend it. I'm anxious to convert my cobbled-together text metadata parser to use your load framework, which will be easier to do if the code is in master. http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java File fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java: http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java@129 PS2, Line 129: queryStmt_.collectTableRefs(allReferencedTables); Will this filter out duplicate table references: SELECT ... FROM foo a, foo b? Should this collection be a Set or Map instead? http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java@164 PS2, Line 164: result.setDbs(new ArrayList<>(uniqueDbs.values())); The values of a map or set are in non-deterministic order. Will it help tests, etc. to sort these entries by name (say) so that the same operation will product the same result? http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/common/JniUtil.java File fe/src/main/java/org/apache/impala/common/JniUtil.java: http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/common/JniUtil.java@120 PS2, Line 120: return serializer.toString(input); If this is for the test case, the resulting string could be very large. Should this take a Writer instead? If you do want a string, you can pass a StringWriter. -- To view, visit http://gerrit.cloudera.org:8080/12221 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iec83eeb2dc5136768b70ed581fb8d3ed0335cb52 Gerrit-Change-Number: 12221 Gerrit-PatchSet: 2 Gerrit-Owner: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Balazs Jeszenszky <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Greg Rahn <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Comment-Date: Wed, 23 Jan 2019 22:14:08 +0000 Gerrit-HasComments: Yes
