[GitHub] [calcite] julianhyde commented on a diff in pull request #2878: [CALCITE-5241] Implement CHAR function

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

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


##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -462,6 +462,16 @@ public static ByteString right(ByteString s, int n) {
     return s.substring(len - n);
   }
 
+  /**
+   * SQL CHAR(int) function.
+   */

Review Comment:
   explain the difference between this and `chr`. It isn't int vs long.



##########
core/src/main/java/org/apache/calcite/sql/type/ReturnTypes.java:
##########
@@ -347,6 +347,13 @@ public static SqlCall stripSeparator(SqlCall call) {
   public static final SqlReturnTypeInference CHAR =
           explicit(SqlTypeName.CHAR);
 
+  /**
+   * Type-inference strategy whereby the result type of a call is a nullable
+   * Char.
+   */
+  public static final SqlReturnTypeInference CHAR_FORCE_NULLABLE =
+      CHAR.andThen(SqlTypeTransforms.FORCE_NULLABLE);
+

Review Comment:
   not 'nullable Char', but 'nullable CHAR(1)'



##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -1492,9 +1492,6 @@ protected static Calendar getCalendarNotTooNear(int timeUnit) {
     f.checkScalar("{fn ASCII('ABC')}", "65", "INTEGER NOT NULL");
     f.checkNull("{fn ASCII(cast(null as varchar(1)))}");
 
-    if (false) {
-      f.checkScalar("{fn CHAR(code)}", null, "");
-    }

Review Comment:
   Why remove this test? The goal of the change is to implement JDBC '{fn CHAR}'



##########
core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java:
##########
@@ -650,7 +650,16 @@ private SqlLibraryOperators() {
           ReturnTypes.BIGINT_NULLABLE, null, OperandTypes.TIMESTAMP,
           SqlFunctionCategory.TIMEDATE);
 
-  @LibraryOperator(libraries = {ORACLE})
+  @LibraryOperator(libraries = {MYSQL, SPARK})
+  public static final SqlFunction CHAR =
+      new SqlFunction("CHAR",
+          SqlKind.OTHER_FUNCTION,
+          ReturnTypes.CHAR_FORCE_NULLABLE,
+          null,
+          OperandTypes.INTEGER,
+          SqlFunctionCategory.STRING);
+
+  @LibraryOperator(libraries = {ORACLE, POSTGRESQL})

Review Comment:
   CHR and CHAR need javadoc



##########
core/src/main/java/org/apache/calcite/util/BuiltInMethod.java:
##########
@@ -339,6 +339,7 @@ public enum BuiltInMethod {
   UPPER(SqlFunctions.class, "upper", String.class),
   LOWER(SqlFunctions.class, "lower", String.class),
   ASCII(SqlFunctions.class, "ascii", String.class),
+  CHAR(SqlFunctions.class, "charN", int.class),
   REPEAT(SqlFunctions.class, "repeat", String.class, int.class),

Review Comment:
   should this be `CHAR(SqlFunctions.class, "charN", long.class)`?
   
   and don't you also need `CHR(SqlFunctions.class, "chr", long.class)`?



##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -462,6 +462,16 @@ public static ByteString right(ByteString s, int n) {
     return s.substring(len - n);
   }
 
+  /**
+   * SQL CHAR(int) function.
+   */

Review Comment:
   also make the javadoc consistent with other javadoc



-- 
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