[GitHub] [beam] apilloud commented on a diff in pull request #24013: Eliminate CalciteUtil.CharType logical type

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

apilloud commented on code in PR #24013:
URL: https://github.com/apache/beam/pull/24013#discussion_r1015877100


##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/utils/CalciteUtils.java:
##########
@@ -109,7 +99,7 @@ public static boolean isStringType(FieldType fieldType) {
   public static final FieldType BOOLEAN = FieldType.BOOLEAN;
   public static final FieldType VARBINARY = FieldType.BYTES;
   public static final FieldType VARCHAR = FieldType.STRING;
-  public static final FieldType CHAR = FieldType.logicalType(new CharType());
+  public static final FieldType CHAR = FieldType.STRING;

Review Comment:
   I disagree. CHAR is not the same as string, it should have fixed length. What breaks if you make this fixed length?



##########
sdks/java/core/src/test/java/org/apache/beam/sdk/util/RowJsonTest.java:
##########
@@ -132,14 +132,10 @@ private static Object[] makeFlatRowTestCase() {
 
     private static Object[] makeLogicalTypeTestCase() {
       Schema schema =
-          Schema.builder()
-              .addLogicalTypeField(
-                  "f_passThroughString",
-                  new PassThroughLogicalType<String>(
-                      "SqlCharType", FieldType.STRING, "", FieldType.STRING) {})
-              .build();
+          Schema.builder().addLogicalTypeField("f_passThroughString", FixedString.of(10)).build();

Review Comment:
   Why `FixedString.of(10)`? Does the padding behavior need to be tested here? (The common use case is in a SQL statement with string constants, which are allocated at the correct size and not padded.)



-- 
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: github-unsubscribe@beam.apache.org

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