xuzifu666 commented on code in PR #5104:
URL: https://github.com/apache/calcite/pull/5104#discussion_r3592232738
##########
core/src/test/java/org/apache/calcite/sql/type/SqlTypeFactoryTest.java:
##########
@@ -87,6 +87,42 @@ class SqlTypeFactoryTest {
assertThat(leastRestrictive.isNullable(), is(true));
}
+ /** UNKNOWN types in leastRestrictive() affect only the result nullability.
*/
+ @Test void testLeastRestrictiveWithUnknown() {
Review Comment:
The current test only covers a single `UNKNOWN` case; could we consider
adding a test case that mixes multiple `UNKNOWN`s with known types, for example:
```
RelDataType r = f.typeFactory.leastRestrictive(
Lists.newArrayList(f.sqlUnknown, f.sqlUnknown, f.sqlBigInt));
assertThat(r.getFullTypeString(), is(f.sqlBigInt.getFullTypeString()));
```
This explicitly guarantees that any number of UNKNOWNs at any position will
not affect the result.
Of course, this is just my understanding; feel free to take it into account.
##########
core/src/main/java/org/apache/calcite/sql/type/SqlTypeFactoryImpl.java:
##########
@@ -191,11 +192,33 @@ public SqlTypeFactoryImpl(RelDataTypeSystem typeSystem) {
RelDataType type0 = types.get(0);
if (type0.getSqlTypeName() != null) {
- RelDataType resultType = leastRestrictiveSqlType(types);
- if (resultType != null) {
- return resultType;
+ // First preprocess to filter out UNKNOWN types.
+ // leastRestrictive() can be thought as a form of type unification,
+ // and UNKNOWN behaves like an unbound type variable: it unifies with
any type
+ // without constraining the result.
+ // Note that UNKNOWN can be nullable, so this information is carried
over to the result.
+ List<RelDataType> knownTypes = new ArrayList<>(types.size());
+ // True if any UNKNONW type is nullable
Review Comment:
UNKNONW -> UNKNOWN
--
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]
