You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/11/02 15:49:00 UTC
[GitHub] [calcite] wnob opened a new pull request #2595: [CALCITE-4872] Make UNKNOWN distinct from NULL (Will Noble)
wnob opened a new pull request #2595:
URL: https://github.com/apache/calcite/pull/2595
Make the unknown `RelDataType` distinct from the null `RelDataType`, and make the casting rules logical. See [CALCITE-4872](https://issues.apache.org/jira/browse/CALCITE-4872).
--
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
[GitHub] [calcite] zinking commented on pull request #2595: [CALCITE-4872] Make new UNKNOWN SqlTypeName, distinct from NULL (Will Noble)
Posted by GitBox <gi...@apache.org>.
zinking commented on pull request #2595:
URL: https://github.com/apache/calcite/pull/2595#issuecomment-957988956
these changes look all pure theoretical, is there real cases , sqls regarding unknown type?
--
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
[GitHub] [calcite] wnob commented on pull request #2595: [CALCITE-4872] Make new UNKNOWN SqlTypeName, distinct from NULL (Will Noble)
Posted by GitBox <gi...@apache.org>.
wnob commented on pull request #2595:
URL: https://github.com/apache/calcite/pull/2595#issuecomment-962137173
Yes, unfortunately Looker makes liberal use of `UNKNOWN`-typed expressions to represent arbitrary user-provided snippets of SQL. With a recent change making use of new rewrite rules, we ran into the issue of `UNKNOWN` types changing to `NULL` types when calling `createWithNullability`, and the rewrite rules failing.
--
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
[GitHub] [calcite] wnob commented on pull request #2595: [CALCITE-4872] Make new UNKNOWN SqlTypeName, distinct from NULL (Will Noble)
Posted by GitBox <gi...@apache.org>.
wnob commented on pull request #2595:
URL: https://github.com/apache/calcite/pull/2595#issuecomment-962137173
Yes, unfortunately Looker makes liberal use of `UNKNOWN`-typed expressions to represent arbitrary user-provided snippets of SQL. With a recent change making use of new rewrite rules, we ran into the issue of `UNKNOWN` types changing to `NULL` types when calling `createWithNullability`, and the rewrite rules failing.
--
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
[GitHub] [calcite] vlsi commented on a change in pull request #2595: [CALCITE-4872] Make new UNKNOWN SqlTypeName, distinct from NULL (Will Noble)
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2595:
URL: https://github.com/apache/calcite/pull/2595#discussion_r753655898
##########
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeUtilTest.java
##########
@@ -226,4 +227,29 @@ private void compareTypesIgnoringNullability(
compareTypesIgnoringNullability("identical types should return true.",
bigIntType, bigIntType1, true);
}
+
+ @Test public void testCanAlwaysCastToUnknownFromBasic() {
+ RelDataType unknownType = f.typeFactory.createUnknownType();
+ RelDataType nullableUnknownType = f.typeFactory.createTypeWithNullability(unknownType, true);
+
+ for (SqlTypeName fromTypeName : SqlTypeName.values()) {
+ BasicSqlType fromType;
+ try {
+ // This only works for basic types. Ignore the rest.
+ fromType = (BasicSqlType) f.typeFactory.createSqlType(fromTypeName);
+ } catch (AssertionError e) {
+ continue;
+ }
+ BasicSqlType nullableFromType = fromType.createWithNullability(!fromType.isNullable);
+
+ assertTrue(SqlTypeUtil.canCastFrom(unknownType, fromType, false));
Review comment:
I would rather suggest refactoring the test into `@ParameterizedTest` or creating a small `assertCanCastFrom(...)`, so the code becomes less noisy, and `assertCanCastFrom` could have clarification messages that explain which case failed (what was the input)
--
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
[GitHub] [calcite] julianhyde commented on a change in pull request #2595: [CALCITE-4872] Make new UNKNOWN SqlTypeName, distinct from NULL (Will Noble)
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2595:
URL: https://github.com/apache/calcite/pull/2595#discussion_r753511546
##########
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeUtilTest.java
##########
@@ -226,4 +227,29 @@ private void compareTypesIgnoringNullability(
compareTypesIgnoringNullability("identical types should return true.",
bigIntType, bigIntType1, true);
}
+
+ @Test public void testCanAlwaysCastToUnknownFromBasic() {
+ RelDataType unknownType = f.typeFactory.createUnknownType();
+ RelDataType nullableUnknownType = f.typeFactory.createTypeWithNullability(unknownType, true);
+
+ for (SqlTypeName fromTypeName : SqlTypeName.values()) {
+ BasicSqlType fromType;
+ try {
+ // This only works for basic types. Ignore the rest.
+ fromType = (BasicSqlType) f.typeFactory.createSqlType(fromTypeName);
+ } catch (AssertionError e) {
+ continue;
+ }
+ BasicSqlType nullableFromType = fromType.createWithNullability(!fromType.isNullable);
+
+ assertTrue(SqlTypeUtil.canCastFrom(unknownType, fromType, false));
Review comment:
use `assertThat(... is(true))`
##########
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeFactoryTest.java
##########
@@ -286,4 +288,18 @@ private void checkCreateSqlTypeWithPrecision(
assertThat(tsWithPrecision3 == tsWithPrecision8, is(true));
}
+ /** Test that the {code UNKNOWN} type does not does not change class when nullified. */
+ @Test void testUnknownCreateWithNullabiliityTypeConsistency() {
Review comment:
typo 'Nullabiliity'
##########
File path: core/src/main/java/org/apache/calcite/sql/type/BasicSqlType.java
##########
@@ -54,7 +54,11 @@
* @param typeName Type name
*/
public BasicSqlType(RelDataTypeSystem typeSystem, SqlTypeName typeName) {
- this(typeSystem, typeName, false, PRECISION_NOT_SPECIFIED,
Review comment:
I am surprised that a basic type can be nullable.
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java
##########
@@ -786,7 +786,7 @@ private SqlNode callToSql(@Nullable RexProgram program, RexCall call0,
SqlNode fieldOperand = field(ordinal);
return SqlStdOperatorTable.CURSOR.createCall(SqlParserPos.ZERO, fieldOperand);
}
- if (ignoreCast) {
+ if (ignoreCast || call.getType().getSqlTypeName() == SqlTypeName.UNKNOWN) {
Review comment:
need comments to justify this. explain to your audience that UNKNOWN type means. it's not trivial.
--
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
[GitHub] [calcite] wnob commented on a change in pull request #2595: [CALCITE-4872] Make new UNKNOWN SqlTypeName, distinct from NULL (Will Noble)
Posted by GitBox <gi...@apache.org>.
wnob commented on a change in pull request #2595:
URL: https://github.com/apache/calcite/pull/2595#discussion_r754893129
##########
File path: core/src/main/java/org/apache/calcite/sql/type/BasicSqlType.java
##########
@@ -54,7 +54,11 @@
* @param typeName Type name
*/
public BasicSqlType(RelDataTypeSystem typeSystem, SqlTypeName typeName) {
- this(typeSystem, typeName, false, PRECISION_NOT_SPECIFIED,
Review comment:
I'm not 100% up to speed on the theoretical ramifications of this, but `BasicSqlType.createWithNullability` is already a thing (and it's existing usage in Calcite is essentially the motivation for this PR).
--
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
[GitHub] [calcite] vlsi commented on a change in pull request #2595: [CALCITE-4872] Make new UNKNOWN SqlTypeName, distinct from NULL (Will Noble)
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2595:
URL: https://github.com/apache/calcite/pull/2595#discussion_r753655898
##########
File path: core/src/test/java/org/apache/calcite/sql/type/SqlTypeUtilTest.java
##########
@@ -226,4 +227,29 @@ private void compareTypesIgnoringNullability(
compareTypesIgnoringNullability("identical types should return true.",
bigIntType, bigIntType1, true);
}
+
+ @Test public void testCanAlwaysCastToUnknownFromBasic() {
+ RelDataType unknownType = f.typeFactory.createUnknownType();
+ RelDataType nullableUnknownType = f.typeFactory.createTypeWithNullability(unknownType, true);
+
+ for (SqlTypeName fromTypeName : SqlTypeName.values()) {
+ BasicSqlType fromType;
+ try {
+ // This only works for basic types. Ignore the rest.
+ fromType = (BasicSqlType) f.typeFactory.createSqlType(fromTypeName);
+ } catch (AssertionError e) {
+ continue;
+ }
+ BasicSqlType nullableFromType = fromType.createWithNullability(!fromType.isNullable);
+
+ assertTrue(SqlTypeUtil.canCastFrom(unknownType, fromType, false));
Review comment:
I would rather suggest refactoring the test into `@ParameterizedTest` or creating a small `assertCanCastFrom(...)`, so the code becomes less noisy, and `assertCanCastFrom` could have clarification messages that explain which case failed (what was the input)
--
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
[GitHub] [calcite] zinking commented on pull request #2595: [CALCITE-4872] Make new UNKNOWN SqlTypeName, distinct from NULL (Will Noble)
Posted by GitBox <gi...@apache.org>.
zinking commented on pull request #2595:
URL: https://github.com/apache/calcite/pull/2595#issuecomment-957988956
these changes look all pure theoretical, is there real cases , sqls regarding unknown type?
--
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
[GitHub] [calcite] julianhyde closed pull request #2595: [CALCITE-4872] Make new UNKNOWN SqlTypeName, distinct from NULL (Will Noble)
Posted by GitBox <gi...@apache.org>.
julianhyde closed pull request #2595:
URL: https://github.com/apache/calcite/pull/2595
--
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
[GitHub] [calcite] wnob commented on pull request #2595: [CALCITE-4872] Make new UNKNOWN SqlTypeName, distinct from NULL (Will Noble)
Posted by GitBox <gi...@apache.org>.
wnob commented on pull request #2595:
URL: https://github.com/apache/calcite/pull/2595#issuecomment-962137173
Yes, unfortunately Looker makes liberal use of `UNKNOWN`-typed expressions to represent arbitrary user-provided snippets of SQL. With a recent change making use of new rewrite rules, we ran into the issue of `UNKNOWN` types changing to `NULL` types when calling `createWithNullability`, and the rewrite rules failing.
--
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