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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
