Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/22333 )
Change subject: IMPALA-13903: Enable hooks in test framework to run parser tests for Calcite planner ...................................................................... Patch Set 5: (4 comments) http://gerrit.cloudera.org:8080/#/c/22333/5/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/22333/5/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@47 PS5, Line 47: public final static String[] operands_ = Nit: I think this can stay private http://gerrit.cloudera.org:8080/#/c/22333/5/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java: http://gerrit.cloudera.org:8080/#/c/22333/5/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java@93 PS5, Line 93: : protected final ParserFixture parserFixture_; : : public FrontendTestBase() { : this(new CupParserFixture()); : } : : public FrontendTestBase(ParserFixture parserFixture) { : parserFixture_ = parserFixture; : } I don't think ParserFixture makes sense as an argument here. The problem with a ParserFixture here is that the analysis phase needs to match the parsing phase. Something with a granularity smaller than CompilerFactory requires extra work to make sure that they match. This would impact FrontendFixture's parseAndAnalyze(), which would be mixing a Calcite ParserFixture with a non-Calcite CompilerFactory. If the Calcite ParserFixture falls back to the non-Calcite parser, how would something know to use the non-Calcite analysis phase? If the Calcite ParserFixture doesn't fall back, then it can't work for CREATE VIEW, etc. If a test wants to supply something, it should be a CompilerFactory or larger. ParserFixture has a bunch of stuff that is useful to ParserTest, but it doesn't really have anything interesting to the rest of the tests. The only things I see are ParsesOk() and the hintStyles_ stuff. ParsesOk() is just the CompilerFactory's createParsedStatement(). The hintStyles_ stuff is static and can be accessed without this instance. I think what we really want is for FrontendTestBase/FrontendFixture to have a pair of CompilerFactory instances. One is the compiler under test that is used for typical workloads (e.g. AnalayzesOk()) and could be either regular or Calcite based on the environment variable. The other is always regular, and it should be used for things like addTestTable/addTestView that can't work with Calcite. http://gerrit.cloudera.org:8080/#/c/22333/5/fe/src/test/java/org/apache/impala/common/ParserFixture.java File fe/src/test/java/org/apache/impala/common/ParserFixture.java: http://gerrit.cloudera.org:8080/#/c/22333/5/fe/src/test/java/org/apache/impala/common/ParserFixture.java@65 PS5, Line 65: public void TestJoinHints(String stmt, String... expectedHints); : : public void TestTableHints(String stmt, String... expectedHints); : : public void TestTableAndJoinHints(String stmt, String... expectedHints); : : public void TestSelectListHints(String stmt, String... expectedHints); : : public void TestInsertAndCtasHints(String insertPart, String ctasPart, : String[] hintStyle, String hints, String... expectedHints); : : public void TestInsertStmtHints(String pattern, String hint, String... expectedHints); : : public void ParserErrorOnInsertStmtHints(String pattern, String hint); : : public void TestCtasHints(String stmt, String... expectedHints); Rather than moving the entire test function out of ParserTest, I would move the individual validation pieces. i.e. We can have functions like validateJoinHints(ParsedStatement, String... expectedHints). http://gerrit.cloudera.org:8080/#/c/22333/5/fe/src/test/java/org/apache/impala/common/ParserFixture.java@82 PS5, Line 82: public void validateCompoundExpr(Object parserReturnObject); Nit: Might as well use ParsedStatement -- To view, visit http://gerrit.cloudera.org:8080/22333 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibbd272f9c2b3ecbb87fa19caccf4fc51a25fea1b Gerrit-Change-Number: 22333 Gerrit-PatchSet: 5 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:15 +0000 Gerrit-HasComments: Yes
