zabetak commented on code in PR #3445:
URL: https://github.com/apache/calcite/pull/3445#discussion_r1339733974
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -13038,9 +13050,19 @@ public TesterImpl() {
super.check(factory, query, typeChecker, parameterChecker,
resultChecker);
final RelDataTypeSystem typeSystem =
factory.typeSystemTransform.apply(RelDataTypeSystem.DEFAULT);
+ // Create a fake reader so we can invoke the getParser() method.
+ // We don't plan to invoke the parser, so we just pass an empty string.
+ Reader reader = new StringReader("");
+ String parserClass = factory
+ .parserConfig()
+ .parserFactory()
+ .getParser(reader)
+ .getClass()
+ .getCanonicalName();
final ConnectionFactory connectionFactory =
factory.connectionFactory
- .with(CalciteConnectionProperty.TYPE_SYSTEM, uri(FIELD));
+ .with(CalciteConnectionProperty.TYPE_SYSTEM, uri(FIELD))
+ .with(CalciteConnectionProperty.PARSER_FACTORY, parserClass +
"#FACTORY");
Review 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.
##########
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:
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?
--
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]
