[CALCITE-7807] Reuse unchanged SQL types during least-restrictive inference

[FrankChen021]

Fixes CALCITE-7807.

Why

During least-restrictive character-type inference, Calcite can apply a charset and collation that a BasicSqlType already has. createWithCharsetAndCollation nevertheless creates an equivalent BasicSqlType and SerializableCharset, after which type-factory canonicalization returns the original instance.

In Druid's string-IN planning benchmark, avoiding this redundant construction reduced allocation by 9.05% at 100,000 literals and 8.72% at 1,000,000 literals. Timing confidence intervals overlapped, so no latency improvement is claimed.

What

Before: already-correct BasicSqlType -> allocate equivalent type -> canonize -> original type
After:  already-correct BasicSqlType -> return this -> canonize -> original type

Return the current immutable type when the requested charset is equal and the collation is the same instance. Collation identity is intentional because SqlCollation.equals does not distinguish coercibility. Requests that change either attribute retain the existing construction and canonicalization path.

This is an unchanged-attribute fast path, not a cache; it neither depends on a previous invocation nor retains derived types.

Verification

• SqlTypeFactoryTest covers unchanged attributes, a different charset, and a collation that differs only in coercibility.
• ./gradlew :core:test --tests org.apache.calcite.sql.type.SqlTypeFactoryTest :core:autostyleJavaCheck :core:checkstyleMain :core:checkstyleTest (27 completed, 0 failed)
• Druid InPlanningBenchmark.queryStringInSqlPlanOnly with -prof gc

Druid benchmark results

String literals	Allocation before	Allocation after	Reduction
100,000	1,450.76 MiB/op	1,319.51 MiB/op	131.24 MiB/op (9.05%)
1,000,000	14,703.47 MiB/op	13,421.72 MiB/op	1,281.76 MiB/op (8.72%)

Configuration: inSubQueryThreshold=2147483647, rowsPerSegment=500000, 2 forks, 2 one-second warmup iterations, and 5 one-second measurement iterations. Allocation is cumulative bytes per operation, not retained or peak heap.

The performance measurement currently comes from Druid; a Calcite-local ubenchmark is not yet included.

Scope

BasicSqlType is public and non-final, so an unchanged request now preserves a subclass instance instead of returning a plain canonical BasicSqlType. Calcite has no such subclasses, and createWithNullability already follows the same pattern. The pre-existing omission of collation coercibility from type canonicalization is outside this change.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[julianhyde]

-1 slop

[vlsi]

The change is correct as far as I can tell. createWithCharsetAndCollation has one production caller, SqlTypeFactoryImpl.createTypeWithCharsetAndCollation, and that caller passes the result to canonize. So canonize(this) returns the same interned instance that canonize(new ...) returned before, and only the allocation changes. I confirmed that the new test fails on 38413ec without the fix, at the first assertSame.

I'd like a few changes before merging; the code-level ones are inline.

Commit message. The commit subject says [CALCITE-7805], but this PR and the summary match CALCITE-7807. CALCITE-7805 is the umbrella issue for allocations in large string ARRAYs. Please rename the commit to [CALCITE-7807] Reuse unchanged SQL types during least-restrictive inference.

Subclasses. BasicSqlType is public and not final. Before this change, createWithCharsetAndCollation always returned a plain BasicSqlType. Now, when the attributes are unchanged, it returns a subclass instance as it is. Calcite has no subclasses, and createWithNullability already behaves this way, so I'm fine with it. Still, please mention it in the description.

Description. It still contains notes from earlier iterations: "The removed no-op SqlTypeFactoryImpl refactoring was excluded" and "based directly on main at 38413ece6". Please drop them.

A related issue that exists before this PR. The digest of a character type includes neither coercibility nor the IMPLICIT and COERCIBLE collations, so canonize returns whichever variant was interned first. Through the factory, createTypeWithCharsetAndCollation(VARCHAR NOT NULL /* IMPLICIT */, ISO-8859-1, SqlCollation.COERCIBLE) returns the same IMPLICIT instance. This PR does not make that worse. It does mean that "Execution Path 1" in the description allocates the derived type and then discards it, and that in path 2 the common type may already be COERCIBLE, in which case the fast path does not apply. I think this deserves its own JIRA issue, since it is about correctness rather than allocation.

[vlsi]

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:

Suggested change

	if (collation == this.collation && Objects.equals(charset, getCharset())) {
	// SqlCollation.equals ignores coercibility (IMPLICIT.equals(COERCIBLE)),
	// so compare collations by reference.
	if (collation == this.collation && charset.equals(getCharset())) {

[vlsi]

Note: it might be we should add something like SqlCollation#isSameAs(SqlCollation) so we compare the collation instead of "identity only" or add coercibility to equals/hashCode. Both changes require analysis.

[FrankChen021]

for the Objects.equals comment here, that's true the charset is not null, however, getCharset is declared as Nullable, the CheckerFramework reports error for the change you propose. So Objects.equals would be the most likely concise statement here.

[vlsi]

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

  /** 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 + ")");
  }

[vlsi]

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.

[vlsi]

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:

  /** 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());
  }

[FrankChen021]

@vlsi Thanks for the review. Let me address these comments.

[FrankChen021]

@vlsi Thanks again for your detailed feedback.

I have addressed the review comments:

• Rewrote the commit message with the correct CALCITE-7807 issue and rationale.
• Documented why collation must be compared by identity.
• Split the test into focused cases covering unchanged attributes, a changed charset, and changed coercibility.
• Added assertions for the resulting charset and collation.
• The canonicalization concern remains outside this PR.
• For SqlCollation#isSameAs, if you agree that we add a method in the SqlCollation to replace current reference comparision, I can update to include it as:

public boolean isSameAs(@Nullable SqlCollation that) {
  return that != null
      && equals(that)
      && coercibility == that.coercibility;
}

Let me know if you have other comments.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud