Re: [PR] [CALCITE-7807] Reuse unchanged SQL types during least-restrictive inference [calcite]

["vlsi (via GitHub)" <gi...@apache.org>]

vlsi commented on code in PR #5281:
URL: https://github.com/apache/calcite/pull/5281#discussion_r4079178228


##########
core/src/main/java/org/apache/calcite/sql/type/BasicSqlType.java:
##########
@@ -156,6 +157,9 @@ public BasicSqlType createWithNullability(boolean nullable) {
   BasicSqlType createWithCharsetAndCollation(Charset charset,
       SqlCollation collation) {
     checkArgument(SqlTypeUtil.inCharFamily(this));
+    if (collation == this.collation && Objects.equals(charset, getCharset())) {

Review Comment:
   `SqlCollation.equals` compares only `collationName` and ignores coercibility, so `SqlCollation.IMPLICIT.equals(SqlCollation.COERCIBLE)` is `true`. If someone later replaces `==` with `Objects.equals(...)`, a request for `COERCIBLE` returns the `IMPLICIT` type. I tried that change: the test catches it only through the `COERCIBLE` case, which does not say that this is what it guards. A one-line comment keeps the next person from making that change.
   
   `charset` is never null here, so `Objects.equals` is not needed either, and the `java.util.Objects` import can go:
   
   ```suggestion
       // SqlCollation.equals ignores coercibility (IMPLICIT.equals(COERCIBLE)),
       // so compare collations by reference.
       if (collation == this.collation && charset.equals(getCharset())) {
   ```



##########
core/src/test/java/org/apache/calcite/sql/type/SqlTypeFactoryTest.java:
##########
@@ -46,6 +50,23 @@
  */
 class SqlTypeFactoryTest {
 
+  @Test void testReuseUnchangedCharsetAndCollation() {
+    SqlTypeFixture f = new SqlTypeFixture();
+    BasicSqlType type = (BasicSqlType) f.sqlVarchar;
+    BasicSqlType decorated =
+        type.createWithCharsetAndCollation(StandardCharsets.UTF_8, SqlCollation.IMPLICIT);
+    assertSame(
+        decorated, decorated.createWithCharsetAndCollation(
+        StandardCharsets.UTF_8, SqlCollation.IMPLICIT));
+    assertNotSame(
+        decorated, decorated.createWithCharsetAndCollation(
+        StandardCharsets.UTF_16, SqlCollation.IMPLICIT));

Review Comment:
   `assertNotSame` alone passes for a method that returns a new object with the old charset. Please assert that the result has `UTF_16` as its charset and `IMPLICIT` as its collation.



##########
core/src/test/java/org/apache/calcite/sql/type/SqlTypeFactoryTest.java:
##########
@@ -46,6 +50,23 @@
  */
 class SqlTypeFactoryTest {
 
+  @Test void testReuseUnchangedCharsetAndCollation() {
+    SqlTypeFixture f = new SqlTypeFixture();
+    BasicSqlType type = (BasicSqlType) f.sqlVarchar;
+    BasicSqlType decorated =
+        type.createWithCharsetAndCollation(StandardCharsets.UTF_8, SqlCollation.IMPLICIT);
+    assertSame(
+        decorated, decorated.createWithCharsetAndCollation(
+        StandardCharsets.UTF_8, SqlCollation.IMPLICIT));
+    assertNotSame(
+        decorated, decorated.createWithCharsetAndCollation(
+        StandardCharsets.UTF_16, SqlCollation.IMPLICIT));
+    BasicSqlType coercible =
+        decorated.createWithCharsetAndCollation(StandardCharsets.UTF_8, SqlCollation.COERCIBLE);
+    assertNotSame(decorated, coercible);
+    assertSame(SqlCollation.COERCIBLE, coercible.getCollation());

Review Comment:
   This is the case that pins `==` over `equals`, since `SqlCollation.COERCIBLE.equals(SqlCollation.IMPLICIT)` is `true`. Please move it into its own test named after that, and assert the charset of the result too:
   
   ```java
     /** SqlCollation.equals ignores coercibility, so COERCIBLE equals IMPLICIT;
      * the type must still be rebuilt. */
     @Test void testCollationDifferingOnlyInCoercibilityIsNotReused() {
       final BasicSqlType type = (BasicSqlType) new SqlTypeFixture().sqlVarchar;
       final Charset charset = requireNonNull(type.getCharset());
       final BasicSqlType coercible =
           type.createWithCharsetAndCollation(charset, SqlCollation.COERCIBLE);
       assertSame(SqlCollation.COERCIBLE, coercible.getCollation());
       assertEquals(charset, coercible.getCharset());
     }
   ```



##########
core/src/test/java/org/apache/calcite/sql/type/SqlTypeFactoryTest.java:
##########
@@ -46,6 +50,23 @@
  */
 class SqlTypeFactoryTest {
 
+  @Test void testReuseUnchangedCharsetAndCollation() {

Review Comment:
   This method checks several scenarios, so the first failure hides the rest, and a failure prints only `expected: not same but was: <VARCHAR>` without naming the call. I suggest one test per case:
   
   - Unchanged attributes return the same instance. Start from `f.sqlVarchar`, which already has the default charset (ISO-8859-1) and `IMPLICIT`. That is the path from the benchmark; this test goes through UTF-8 instead.
   - A collation that differs only in coercibility is not reused (see the comment below).
   - A different charset produces a new type with that charset (see the comment below).
   
   Please also add a message that names the call and the usual `Test case for [CALCITE-7807]` Javadoc. For example (not compiled; needs `Charset`, `requireNonNull`, and `assertEquals` imports):
   
   ```java
     /** Test case for
      * <a href="https://issues.apache.org/jira/browse/CALCITE-7807">[CALCITE-7807]
      * Reuse unchanged SQL types during least-restrictive inference</a>. */
     @Test void testUnchangedCharsetAndCollationReturnsSameType() {
       final BasicSqlType type = (BasicSqlType) new SqlTypeFixture().sqlVarchar;
       final Charset charset = requireNonNull(type.getCharset());
       final SqlCollation collation = requireNonNull(type.getCollation());
       assertSame(type,
           type.createWithCharsetAndCollation(charset, collation),
           "createWithCharsetAndCollation(" + charset + ", " + collation + ")");
     }
   ```



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