julianhyde commented on code in PR #3141:
URL: https://github.com/apache/calcite/pull/3141#discussion_r1164501619
##########
core/src/main/codegen/templates/Parser.jj:
##########
@@ -4708,14 +4708,40 @@ SqlNode ArrayConstructor() :
{
<ARRAY> { s = span(); }
(
- LOOKAHEAD(1)
- <LPAREN>
- // by sub query "MULTISET(SELECT * FROM T)"
- e = OrderedQueryOrExpr(ExprContext.ACCEPT_QUERY)
- <RPAREN>
Review Comment:
That's a lot of 'validation' code in the parser. That's a code smell. Move
it to the validator.
My hunch is that you don't need the `conformance.allowArrayFunction()` at
all, anywhere. In the validator you can just look up a function called 'array'
in the usual way.
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -3684,6 +3684,35 @@ private SqlDialect nonOrdinalDialect() {
sql(query).withSpark().ok(expected);
}
+ private Sql sqlSpark(String query) {
+ return sql(query)
+
.parserConfig(SqlParser.Config.DEFAULT.withConformance(SqlConformanceEnum.SPARK))
+ .withSpark()
+ .withLibrary(SqlLibrary.SPARK);
+ }
+
+ @Test void testArrayFunction() {
+ sqlSpark("SELECT ARRAY(1, 2)")
+ .ok("SELECT ARRAY(1, 2)\n"
+ + "FROM (VALUES (0)) t (ZERO)");
+ }
+
+ @Test void testArrayFunctionNullary() {
+ sqlSpark("SELECT ARRAY()")
+ .ok("SELECT ARRAY()\n"
+ + "FROM (VALUES (0)) t (ZERO)");
+ }
+
+ @Test void testArrayQueryInvalid() {
Review Comment:
this should be in SqlPArserTest
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java:
##########
@@ -77,6 +77,10 @@ public enum SqlConformanceEnum implements SqlConformance {
* consistent with Presto. */
PRESTO,
+ /** Conformance value that instructs Calcite to use SQL semantics
+ * consistent with Spark. */
Review Comment:
Apache Spark
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -7584,6 +7584,20 @@ private static void
checkArrayConcatAggFuncFails(SqlOperatorFixture t) {
f.checkFails("^Array[]^", "Require at least 1 argument", false);
}
+ @Test void testArrayFunctionConstructor() {
Review Comment:
rename test to `testArrayFunction`. it's not a constructor.
add javadoc making clear that it's for the function in the Spark library.
test what happens when the Spark library is not enabled. (as I said
elsewhere, I think you probably don't need the conformance setting.)
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -7584,6 +7584,20 @@ private static void
checkArrayConcatAggFuncFails(SqlOperatorFixture t) {
f.checkFails("^Array[]^", "Require at least 1 argument", false);
}
+ @Test void testArrayFunctionConstructor() {
+ final SqlOperatorFixture f = fixture();
Review Comment:
add tests where some or all arguments are null
--
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]
