Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12221 )
Change subject: [PROTOTYPE] IMPALA-5872: Test case builder for query planner ...................................................................... Patch Set 1: (8 comments) Very nice. Far simpler than what I cobbled together for a bug I worked on. Is there a way to include the query itself in the test case? That eliminates the question about "did I get the correct data for the query or query for the data?" Will test case loading work even with planner-only test? Or, is a "live" Impala required? http://gerrit.cloudera.org:8080/#/c/12221/1/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/12221/1/fe/src/main/cup/sql-parser.cup@282 PS1, Line 282: KW_ENCODING, KW_END, KW_ESCAPED, KW_EXISTS, KW_EXPLAIN, KW_EXPORT, KW_EXTENDED, Will introducing a keyword using a common term cause existing queries to fail? Or, was EXPORT already reserved? That is, before the change, this was legal: SELECT country, export FROM tradestats; But, after the change the query will require: SELECT country, `export` FROM tradestats; http://gerrit.cloudera.org:8080/#/c/12221/1/fe/src/main/cup/sql-parser.cup@773 PS1, Line 773: KW_EXPORT KW_TESTCASE KW_INTO KW_OUTFILE STRING_LITERAL:path query_stmt:query Insert a keyword before the query to allow future revisions? Eliminate OUTFILE keyword? EXPORT TESTCASE INTO 'foo.json` STORED AS 'json' FOR SELECT ... http://gerrit.cloudera.org:8080/#/c/12221/1/fe/src/main/cup/sql-parser.cup@782 PS1, Line 782: RESULT = new LoadTestCaseStmt(new HdfsUri(path)); What does LOAD mean? Is this a load-and-plan? Suppose I want to do an EXPLAIN at a specific detail level. How would this statement interact with the EXPLAIN? Or, does this just load the metadata? If so, would we have a way to export the query above along with the metadata? http://gerrit.cloudera.org:8080/#/c/12221/1/fe/src/main/java/org/apache/impala/analysis/ExportTestCaseStmt.java File fe/src/main/java/org/apache/impala/analysis/ExportTestCaseStmt.java: http://gerrit.cloudera.org:8080/#/c/12221/1/fe/src/main/java/org/apache/impala/analysis/ExportTestCaseStmt.java@86 PS1, Line 86: table.getLock().lock(); Is the lock needed? None of our other table-related code in the analyzer or planner obtain locks. So, is that code wrong? Or is the lock not needed here? Also, although we lock the table here to prevent change, what prevents the table from having changed since the earlier analysis step? http://gerrit.cloudera.org:8080/#/c/12221/1/fe/src/main/java/org/apache/impala/analysis/ExportTestCaseStmt.java@94 PS1, Line 94: if (!result.getDbs().contains(thriftDb)) { Does this work? contains() uses the 'equals()' method, which, presumably, is generated and checks each field. Better than have a Map of table names and not generate Thrift when the DB already exists in results? http://gerrit.cloudera.org:8080/#/c/12221/1/fe/src/main/java/org/apache/impala/analysis/ExportTestCaseStmt.java@99 PS1, Line 99: Set<FeView> allReferencedViews = new HashSet<>(); This will also be a value set, which means we will compare inline views field-by-field. Is there a more efficient, deterministic way to enumerate them? Perhaps using an IdentityMap (sadly, there is no IdentitySet that I can find.) http://gerrit.cloudera.org:8080/#/c/12221/1/fe/src/main/java/org/apache/impala/analysis/ExportTestCaseStmt.java@101 PS1, Line 101: for (FeView feView: allReferencedViews) { Set iteration has no defined order. Run the same operation twice and the export file may be different due to order. Should we sort the views first by some criteria to ensure determinism? http://gerrit.cloudera.org:8080/#/c/12221/1/fe/src/main/java/org/apache/impala/analysis/ExportTestCaseStmt.java@121 PS1, Line 121: outputPath_.getPath(), testOutputFilePrefix + UUID.randomUUID().toString()); This appends a UUID. The user gave a name. How will the user know the actual name of the file? Would it be better to require the user to specify a non-existing file name? -- 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: 1 Gerrit-Owner: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Balazs Jeszenszky <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Greg Rahn <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Comment-Date: Fri, 18 Jan 2019 03:26:20 +0000 Gerrit-HasComments: Yes
