Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/22334 )
Change subject: IMPALA-13904: Run parser junit tests through Calcite Planner ...................................................................... Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/22334/4/fe/src/test/java/org/apache/impala/analysis/CalciteParserFixture.java File fe/src/test/java/org/apache/impala/analysis/CalciteParserFixture.java: http://gerrit.cloudera.org:8080/#/c/22334/4/fe/src/test/java/org/apache/impala/analysis/CalciteParserFixture.java@238 PS4, Line 238: public ParsedStatement ParsesOk(String stmt) { The Impala idiom is that test infrastructure doesn't change the original truth statement meaningfully. This is taking the original statement from ParserTest and changing it into an entirely different statement that the test author never expressed. In order to know what the true statement is, someone needs to find all the pieces and reconstruct the whole thing. Half of these statements are constructed with String.format(), so it's not easy to put it all together. The truth needs to come from ParserTest itself. ParserTest needs to know about Calcite and address the differences directly. Here's how I would do it: 1. Add an enum of reasons why Calcite can fail when the regular parser would succeed 2. Add an enum of reasons why Calcite can succeed when the regular parser would fail 3. ParsesOk() takes an optional extra argument that specifies the reason for Calcite failing. 4. ParserError() takes an optional extra argument that specifies the reason for Calcite succeeding. 5. When this extra argument is specified and non-null, the Calcite version doesn't skip the test. It flips the expected result. For ParsesOk() with a Calcite failure reason, it turns into ParserError(). For ParserError() with a Calcite success reason, it turns into ParsesOk(). In general, the emphasis is on flipping results as needed rather than skipping things. 7. Add a way to set an expected Calcite failure for a whole block (i.e. pushCalciteErrorReason/popCalciteErrorReason). This can be used for tests for DML/DDLs, etc. 8. No central lists of exceptions. No fallback to the regular parser. http://gerrit.cloudera.org:8080/#/c/22334/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java File fe/src/test/java/org/apache/impala/analysis/ParserTest.java: http://gerrit.cloudera.org:8080/#/c/22334/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@1029 PS4, Line 1029: ParsesOk("select a from default.`t`"); Most of these fail because default is a keyword http://gerrit.cloudera.org:8080/#/c/22334/4/java/calcite-planner/pom.xml File java/calcite-planner/pom.xml: http://gerrit.cloudera.org:8080/#/c/22334/4/java/calcite-planner/pom.xml@39 PS4, Line 39: 5.0.0-SNAPSHOT Also here: Use ${project.version} http://gerrit.cloudera.org:8080/#/c/22334/4/java/calcite-planner/pom.xml@59 PS4, Line 59: 5.0.0-SNAPSHOT Use ${project.version} -- To view, visit http://gerrit.cloudera.org:8080/22334 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I30a2b3422fa4c408dc6766d3ccef0c05f2ecc093 Gerrit-Change-Number: 22334 Gerrit-PatchSet: 4 Gerrit-Owner: Steve Carlin <scar...@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Mon, 28 Apr 2025 22:04:39 +0000 Gerrit-HasComments: Yes
