[GitHub] [calcite] julianhyde commented on a diff in pull request #2819: [CALCITE-5159] Postgres dialect should support implicit cast from string literal to array literal

[GitBox <gi...@apache.org>]

julianhyde commented on code in PR #2819:
URL: https://github.com/apache/calcite/pull/2819#discussion_r883903595


##########
core/src/main/java/org/apache/calcite/sql/SqlLiteral.java:
##########
@@ -995,6 +995,22 @@ public static SqlCharStringLiteral createCharString(
     return new SqlCharStringLiteral(slit, pos);
   }
 
+  /**
+   * Creates composition of a string literal and an array value.
+   *

Review Comment:
   add an example



##########
core/src/main/java/org/apache/calcite/sql/SqlLiteral.java:
##########
@@ -995,6 +995,22 @@ public static SqlCharStringLiteral createCharString(
     return new SqlCharStringLiteral(slit, pos);
   }
 
+  /**
+   * Creates composition of a string literal and an array value.
+   *
+   * @param s           a string (without the sql single quotes)
+   * @param arrayValue  array value gotten from s
+   * @param pos         Parser position
+   * @return A string literal
+   */
+  public static SqlCharStringLiteral createArrayStringLiteral(
+      NlsString s,
+      SqlCall arrayValue,
+      SqlParserPos pos
+  ) {

Review Comment:
   parens must not be at start of line
   
   return is not 'string literal'



##########
core/src/main/codegen/templates/Parser.jj:
##########
@@ -238,6 +238,39 @@ public class ${parser.class} extends SqlAbstractParserImpl
         return SqlStmtList();
     }
 
+    // Q: Is it the best way ???

Review Comment:
   I'd add a method `parseArray` to `SqlAbstractParserImpl`, as a top-level entry point to the parser. 
   
   Also add a `parseArrayLiteral` method in `SqlParserUtil` similar to `parseTimestampLiteral`. The implementation will create a fresh parser then call `parseArray`.
   
   If the validator needs to convert a char literal into an array literal, it can call that method.



##########
core/src/main/java/org/apache/calcite/config/CalciteConnectionConfig.java:
##########
@@ -110,4 +110,6 @@ public interface CalciteConnectionConfig extends ConnectionConfig {
   boolean lenientOperatorLookup();
   /** Returns the value of {@link CalciteConnectionProperty#TOPDOWN_OPT}. */
   boolean topDownOpt();
+  /** Returns the value of {@link CalciteConnectionProperty#TYPE_COERCION_STRING_TO_ARRAY}. */
+  boolean typeCoercionStringToArray();

Review Comment:
   not the best place to configure this. probably type system would be better. or conformance. at the very least mark it experimental. 



##########
babel/src/test/resources/sql/postgresql.iq:
##########
@@ -0,0 +1,59 @@
+# postgresql.iq - Babel test for Postgresql dialect of SQL

Review Comment:
   "PostgreSQL" or "Postgres" not "Postgresql"



##########
core/src/main/codegen/templates/Parser.jj:
##########
@@ -238,6 +238,39 @@ public class ${parser.class} extends SqlAbstractParserImpl
         return SqlStmtList();
     }
 
+    // Q: Is it the best way ???

Review Comment:
   I would not preemptively convert every char literal into an array literal. Which means that `class SqlArrayCharStringLiteral` is probably not needed.



-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org