[CALCITE-6029] SqlOperatorTest cannot test operators that require the Babel parser#3445
[CALCITE-6029] SqlOperatorTest cannot test operators that require the Babel parser#3445mihaibudiu wants to merge 1 commit intoapache:mainfrom
Conversation
… Babel parser Signed-off-by: Mihai Budiu <mbudiu@gmail.com>
|
Kudos, SonarCloud Quality Gate passed! |
| factory.connectionFactory | ||
| .with(CalciteConnectionProperty.TYPE_SYSTEM, uri(FIELD)); | ||
| .with(CalciteConnectionProperty.TYPE_SYSTEM, uri(FIELD)) | ||
| .with(CalciteConnectionProperty.PARSER_FACTORY, parserClass + "#FACTORY"); |
There was a problem hiding this comment.
We are not propagating any other connection property here so not sure why we should add special handle for the PARSER_FACTORY; it seems a bit inconsistent to pass just this and ignore all other parser properties.
If we want to have some kind of default behavior for the fixture maybe it would be more appropriate to put such logic in SqlOperatorFixture and do something similar to org.apache.calcite.sql.test.SqlOperatorFixture#withConformance but for the parser factory.
| @Test void testBabel() { | ||
| final SqlOperatorFixture f = fixture().withLibrary(SqlLibrary.POSTGRESQL) | ||
| .withParserConfig(p -> p.withParserFactory(SqlBabelParserImpl.FACTORY)); | ||
| f.checkScalar("10::integer", | ||
| "10", "INTEGER NOT NULL"); | ||
| } |
There was a problem hiding this comment.
The main focus of this class is testing operators so the test name should adhere to the guidelines present in the main Javadoc of the class. Something like testInfixCast would be more appropriate.
Worth mentioning that any new tests added here will be inherited in every sub-class inside or outside the Calcite repo. Do we really want to include and enforce testing of non-standard (babel) operators everywhere?
Finally, we have already separate classes for testing parsing (BabelParserTest) and execution (BabelTest) of non-standard operators. If we accept Babel operators here how someone in the future will decide where to put their tests?
There was a problem hiding this comment.
That's true, I can change the name. But the point of this test wasn't to test the operator, but to test the fact that parser configuration is propagated to the fixture. Perhaps a better name is testInfixWithBabel.
None of the fixtures in the Babel tests seems to check operator behavior.
There are a few benefits allowing SqlOperatorTest handle Babel:
- simpler to write operator tests, you don't have to think where they belong
- you benefit from all the fixtures and classes that overload this one, e.g., CalciteSqlOperatorTest; I have also sent a new PR for a new fixture: [CALCITE-6014] Create a SqlOperatorFixture that parses, unparses, and… #3433 and I plan to add one more as described in https://issues.apache.org/jira/browse/CALCITE-5891
- the API already seems to allow this, but doesn't work correctly (which is what this issue addresses)
Alternatively, if we don't accept this change, we should build infrastructure similar to SqlOperatorTest for Babel tests (and perhaps for other parsers that people could come up with). We should then also overload the withParserConfig of the fixture to give a warning if someone attempts to change the factory, since it doesn't work correctly.
Note that, although some operators are Babel, their implementation is still in core. Case in point is #3446, which is really why I filed this bug.
Another weird thing is that CalciteSqlOperatorTests is in core, while SqlOperatorTest is in testkit. Because of this there appears a circuilar dependence between modules core -> testkit -> core. This PR would add another loop into the dependency graph. But perhaps that doesn't matter.
There was a problem hiding this comment.
At the moment, I am rather neutral on this PR, don't have a strong opinion about merging it or not.
None of the fixtures in the Babel tests seems to check operator behavior.
Not sure what exactly you mean by operator behavior. I see that it is possible to parse, validate, and execute operators usingBabelParserTestandBabelTest. Which operator behavior we are not able to test?
Alternatively, if we don't accept this change, we should build infrastructure similar to SqlOperatorTest for Babel tests...
Can you elaborate why the existing BabelXx test classes are not sufficient? If it is easy/equivalent to add new tests there then in terms of maintenance it may be better to keep the Babel tests centralized.
If your ultimate goal is to replace/unify those BabelXx classes with SqlOperatorTest then that's a different discussion and I would be very positive to move into that direction.
Note that, although some operators are Babel, their implementation is still in core.
The babel module depends on core so I see no big problem around this.
Another weird thing is that CalciteSqlOperatorTests is in core, while SqlOperatorTest is in testkit
Yes I noticed this as well. It is indeed a bit weird, but since it is already like that I don't mind adding another loop.
|
I will close this PR. The issue is real, but there are workarounds that involve some effort, such as writing operator tests for Babel operators in different test files. |
2 participants
No description provided.