mihaibudiu commented on code in PR #3445:
URL: https://github.com/apache/calcite/pull/3445#discussion_r1340726920
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -9722,6 +9725,15 @@ private static void
checkArrayConcatAggFuncFails(SqlOperatorFixture t) {
false);
}
+ /** Test case for <a
href="https://issues.apache.org/jira/browse/CALCITE-6029";>
+ * [CALCITE-6029] SqlOperatorTest cannot test operators that require the
Babel parser</a>. */
+ @Test void testBabel() {
+ final SqlOperatorFixture f = fixture().withLibrary(SqlLibrary.POSTGRESQL)
+ .withParserConfig(p ->
p.withParserFactory(SqlBabelParserImpl.FACTORY));
+ f.checkScalar("10::integer",
+ "10", "INTEGER NOT NULL");
+ }
Review 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:
#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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
