vlsi commented on code in PR #5281:
URL: https://github.com/apache/calcite/pull/5281#discussion_r4243071869
##########
core/src/test/java/org/apache/calcite/sql/type/SqlTypeFactoryTest.java:
##########
@@ -36,16 +39,57 @@
import static org.hamcrest.CoreMatchers.notNullValue;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.hasToString;
+import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotSame;
import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
+import static java.util.Objects.requireNonNull;
+
/**
* Test for {@link SqlTypeFactoryImpl}.
*/
class SqlTypeFactoryTest {
+ /** 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 + ")");
+ }
+
+ @Test void testDifferentCharsetIsAppliedToNewType() {
+ final BasicSqlType type = (BasicSqlType) new SqlTypeFixture().sqlVarchar;
+ final BasicSqlType utf16Type =
+ type.createWithCharsetAndCollation(StandardCharsets.UTF_16,
SqlCollation.IMPLICIT);
+
+ assertNotSame(type, utf16Type, "a different charset must produce a new
type");
+ assertEquals(StandardCharsets.UTF_16, utf16Type.getCharset());
+ assertSame(SqlCollation.IMPLICIT, utf16Type.getCollation());
+ }
+
+ /** {@link SqlCollation#equals} ignores coercibility, so COERCIBLE equals
+ * IMPLICIT; the type must still be rebuilt. */
+ @Test void testCollationDifferingOnlyInCoercibilityIsNotReused() {
+ final BasicSqlType type = (BasicSqlType) new SqlTypeFixture().sqlVarchar;
Review Comment:
`sqlVarchar` comes from the global `DATATYPE_CACHE`, and the digest omits
`IMPLICIT`/`COERCIBLE`, so the fixture holds whichever variant was interned
first in this JVM. If a `COERCIBLE` `VARCHAR NOT NULL` gets there first, the
fast path returns `this` and the test fails although the code is right; the
precondition "differs only in coercibility" is not asserted. Build the base
type outside the cache so the `IMPLICIT` is visible in the test:
```java
final BasicSqlType type =
new BasicSqlType(RelDataTypeSystem.DEFAULT, SqlTypeName.VARCHAR, 10)
.createWithCharsetAndCollation(StandardCharsets.ISO_8859_1,
SqlCollation.IMPLICIT);
```
##########
core/src/test/java/org/apache/calcite/sql/type/SqlTypeFactoryTest.java:
##########
@@ -36,16 +39,57 @@
import static org.hamcrest.CoreMatchers.notNullValue;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.hasToString;
+import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotSame;
import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
+import static java.util.Objects.requireNonNull;
+
/**
* Test for {@link SqlTypeFactoryImpl}.
*/
class SqlTypeFactoryTest {
+ /** 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 + ")");
+ }
+
+ @Test void testDifferentCharsetIsAppliedToNewType() {
+ final BasicSqlType type = (BasicSqlType) new SqlTypeFixture().sqlVarchar;
+ final BasicSqlType utf16Type =
+ type.createWithCharsetAndCollation(StandardCharsets.UTF_16,
SqlCollation.IMPLICIT);
+
+ assertNotSame(type, utf16Type, "a different charset must produce a new
type");
Review Comment:
The message restates the test name, and the failure prints only `<VARCHAR>`.
The assertions after it already fail if `this` is returned, since the type is
immutable, and they print the charset and collation. Please drop the
`assertNotSame` here and in
`testCollationDifferingOnlyInCoercibilityIsNotReused`.
##########
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:
You are right: with `-PenableCheckerframework` on JDK 11,
`charset.equals(getCharset())` fails with `argument.type.incompatible`. Please
keep `Objects.equals`.
--
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]
