Re: [PR] [FLINK-33171][table planner] Consistent implicit type coercion support for equal and non-equal comparisons for codegen [flink]
LadyForest merged PR #23668: URL: https://github.com/apache/flink/pull/23668 -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33171][table planner] Consistent implicit type coercion support for equal and non-equal comparisons for codegen [flink]
LadyForest merged PR #23669: URL: https://github.com/apache/flink/pull/23669 -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33171][table planner] Consistent implicit type coercion support for equal and non-equal comparisons for codegen [flink]
flinkbot commented on PR #23669: URL: https://github.com/apache/flink/pull/23669#issuecomment-1794180188 ## CI report: * 2779239ff7d889f0a15dd837609dbb900b9bc693 UNKNOWN Bot commands The @flinkbot bot supports the following commands: - `@flinkbot run azure` re-run the last Azure build -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33171][table planner] Consistent implicit type coercion support for equal and non-equal comparisons for codegen [flink]
flinkbot commented on PR #23668: URL: https://github.com/apache/flink/pull/23668#issuecomment-1794179424 ## CI report: * 8cf6ef0c8e877dfc01984cef744382ad6d68ea97 UNKNOWN Bot commands The @flinkbot bot supports the following commands: - `@flinkbot run azure` re-run the last Azure build -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33171][table planner] Consistent implicit type coercion support for equal and non-equal comparisons for codegen [flink]
fengjiajie commented on PR #23478: URL: https://github.com/apache/flink/pull/23478#issuecomment-1794179250 > Hi @fengjiajie, could you pick the fix to branch release-1.18 and release-1.17 as well? Hi @LadyForest , thank you for moving this forward. cherry-picks to 1.17 and 1.18: * https://github.com/apache/flink/pull/23668 * https://github.com/apache/flink/pull/23669 -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33171][table planner] Consistent implicit type coercion support for equal and non-equal comparisons for codegen [flink]
LadyForest commented on PR #23478: URL: https://github.com/apache/flink/pull/23478#issuecomment-1794159224 Hi @fengjiajie, could you pick the fix to branch release-1.18 and release-1.17 as well? -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33171][table planner] Consistent implicit type coercion support for equal and non-equal comparisons for codegen [flink]
LadyForest merged PR #23478: URL: https://github.com/apache/flink/pull/23478 -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33171][table planner] Consistent implicit type coercion support for equal and non-equal comparisons for codegen [flink]
LadyForest commented on PR #23478: URL: https://github.com/apache/flink/pull/23478#issuecomment-1793681676 > @lincoln-lil @LadyForest When you have time, please take a look if any further modifications are needed, thanks. I'm sorry for the late reply; I'll take a look as soon as possible. -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33171][table planner] Consistent implicit type coercion support for equal and non-equal comparisons for codegen [flink]
fengjiajie commented on PR #23478: URL: https://github.com/apache/flink/pull/23478#issuecomment-1792143248 @lincoln-lil @LadyForest When you have time, please take a look if any further modifications are needed, thanks. -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33171][table planner] Consistent implicit type coercion support for equal and non-equal comparisons for codegen [flink]
fengjiajie commented on PR #23478: URL: https://github.com/apache/flink/pull/23478#issuecomment-1776952523 @LadyForest Thank you for helping troubleshoot the issue. -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33171][table planner] Consistent implicit type coercion support for equal and non-equal comparisons for codegen [flink]
LadyForest commented on code in PR #23478:
URL: https://github.com/apache/flink/pull/23478#discussion_r1369833456
##
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala:
##
@@ -403,47 +445,71 @@ object ScalarOperatorGens {
|equals(${right.resultTerm}.getBinarySection());
|}
|""".stripMargin
- GeneratedExpression(resultTerm, nullTerm, code, resultType)
+ wrapExpressionIfNonEq(
Review Comment:
Similarly, here we cannot call `wrapExpressionIfNonEq` directly.
I've composed a test case to reproduce the issue.
```scala
@Test
def debugging(): Unit = {
//You can set the class access modifier to public temporarily for test
val rawType = new RawType[LogicalTypesTest.Human](
classOf[LogicalTypesTest.Human],
new KryoSerializer[LogicalTypesTest.Human](
classOf[LogicalTypesTest.Human],
new ExecutionConfig))
val className = "org.apache.flink.table.types.LogicalTypesTest$Human"
val serializerString = rawType.asSerializableString
tEnv.executeSql(
s"""
|create table test_in1 (
| content raw('$className', '$serializerString')
|) with ('connector' = 'filesystem', 'format' = 'raw', 'path' =
'/tmp/test-dir-in-1')
|""".stripMargin)
tEnv.executeSql(
s"""
|create table test_in2 (
| content raw('$className', '$serializerString')
|) with ('connector' = 'filesystem', 'format' = 'raw', 'path' =
'/tmp/test-dir-in-2')
|""".stripMargin)
tEnv.executeSql(
s"""
|create table test_out (
| content raw('$className', '$serializerString')
|) with ('connector' = 'filesystem', 'format' = 'raw', 'path' =
'/tmp/test-dir-out')
|""".stripMargin)
tEnv
.executeSql(
"""
|insert into test_out
|select c2 from (
| select in1.content as c1, in2.content as c2
| from test_in1 in1 join test_in2 in2
| on in1.content <> in2.content
|) tmp
|""".stripMargin)
.await()
}
```
The correct and wrong generated non-equi condition are as follows:
```java
// correct
boolean isNull$5 = isNull$0 || isNull$3;
boolean result$5 = false;
if (!isNull$5) {
field$2.ensureMaterialized(typeSerializer$1.getInnerSerializer());
field$4.ensureMaterialized(typeSerializer$1.getInnerSerializer());
result$5 =
!field$2.getBinarySection().
equals(field$4.getBinarySection());
}
return result$5;
// wrong
boolean isNull$5 = isNull$0 || isNull$3;
boolean result$5 = false;
if (!isNull$5) {
field$2.ensureMaterialized(typeSerializer$1.getInnerSerializer());
field$4.ensureMaterialized(typeSerializer$1.getInnerSerializer());
result$5 =
field$2.getBinarySection().
equals(field$4.getBinarySection());
}
return (!result$5);
```
##
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala:
##
@@ -345,44 +345,86 @@ object ScalarOperatorGens {
}
}
- def generateEquals(
+ private def wrapExpressionIfNonEq(
+ isNonEq: Boolean,
+ equalsExpr: GeneratedExpression,
+ resultType: LogicalType): GeneratedExpression = {
+if (isNonEq) {
+ GeneratedExpression(
+s"(!${equalsExpr.resultTerm})",
+equalsExpr.nullTerm,
+equalsExpr.code,
+resultType)
+} else {
+ equalsExpr
+}
+ }
+
+ private def generateEqualAndNonEqual(
ctx: CodeGeneratorContext,
left: GeneratedExpression,
right: GeneratedExpression,
+ operator: String,
resultType: LogicalType): GeneratedExpression = {
+
checkImplicitConversionValidity(left, right)
+
+val nonEq = operator match {
+ case "==" => false
+ case "!=" => true
+ case _ => throw new CodeGenException(s"Unsupported boolean comparison
'$operator'.")
+}
val canEqual = isInteroperable(left.resultType, right.resultType)
+
if (isCharacterString(left.resultType) &&
isCharacterString(right.resultType)) {
- generateOperatorIfNotNull(ctx, resultType, left, right)(
-(leftTerm, rightTerm) => s"$leftTerm.equals($rightTerm)")
+ wrapExpressionIfNonEq(
Review Comment:
Add the following test to MiscITCase can reproduce the issue
```scala
@Test
def testCompareNullInFilter(): Unit = {
checkQuery(
Seq((null, 2), ("b", 1)),
"SELECT f1 FROM Table1 WHERE f0 <> 'a'",
Re: [PR] [FLINK-33171][table planner] Consistent implicit type coercion support for equal and non-equal comparisons for codegen [flink]
LadyForest commented on code in PR #23478:
URL: https://github.com/apache/flink/pull/23478#discussion_r1368880765
##
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala:
##
@@ -345,44 +345,86 @@ object ScalarOperatorGens {
}
}
- def generateEquals(
+ private def wrapExpressionIfNonEq(
+ isNonEq: Boolean,
+ equalsExpr: GeneratedExpression,
+ resultType: LogicalType): GeneratedExpression = {
+if (isNonEq) {
+ GeneratedExpression(
+s"(!${equalsExpr.resultTerm})",
+equalsExpr.nullTerm,
+equalsExpr.code,
+resultType)
+} else {
+ equalsExpr
+}
+ }
+
+ private def generateEqualAndNonEqual(
ctx: CodeGeneratorContext,
left: GeneratedExpression,
right: GeneratedExpression,
+ operator: String,
resultType: LogicalType): GeneratedExpression = {
+
checkImplicitConversionValidity(left, right)
+
+val nonEq = operator match {
+ case "==" => false
+ case "!=" => true
+ case _ => throw new CodeGenException(s"Unsupported boolean comparison
'$operator'.")
+}
val canEqual = isInteroperable(left.resultType, right.resultType)
+
if (isCharacterString(left.resultType) &&
isCharacterString(right.resultType)) {
- generateOperatorIfNotNull(ctx, resultType, left, right)(
-(leftTerm, rightTerm) => s"$leftTerm.equals($rightTerm)")
+ wrapExpressionIfNonEq(
Review Comment:
Hi @fengjiajie, I have reconsidered and realized that the equal and
non-equal comparisons of string types cannot directly invoke the
"wrapExpressionIfNonEq" function. For example, in TPC-DS Q19, the generated
join operator's non-equal conditions are as follows:
```java
// right, generated by
// generateOperatorIfNotNull(ctx, resultType, left, right)((leftTerm,
rightTerm) => s"$leftTerm.equals($rightTerm)")
isNull$854 = isNull$851 || false || false;
result$855 = org.apache.flink.table.data.binary.BinaryStringData.EMPTY_UTF8;
if (!isNull$854) {
result$855 =
org.apache.flink.table.data.binary.BinaryStringDataUtil.substringSQL(field$853,
((int) 1), ((int) 5));
isNull$854 = (result$855 == null);
}
isNull$858 = isNull$856 || false || false;
result$859 = org.apache.flink.table.data.binary.BinaryStringData.EMPTY_UTF8;
if (!isNull$858) {
result$859 =
org.apache.flink.table.data.binary.BinaryStringDataUtil.substringSQL(field$857,
((int) 1), ((int) 5));
isNull$858 = (result$859 == null);
}
isNull$860 = isNull$854 || isNull$858;
result$861 = false;
if (!isNull$860) {
result$861 = !result$855.equals(result$859);
}
return result$861;
```
```java
// wrong, generated by wrapExpressionIfNonEq(...)
isNull$854 = isNull$851 || false || false;
result$855 = org.apache.flink.table.data.binary.BinaryStringData.EMPTY_UTF8;
if (!isNull$854) {
result$855 =
org.apache.flink.table.data.binary.BinaryStringDataUtil.substringSQL(field$853,
((int) 1), ((int) 5));
isNull$854 = (result$855 == null);
}
isNull$858 = isNull$856 || false || false;
result$859 = org.apache.flink.table.data.binary.BinaryStringData.EMPTY_UTF8;
if (!isNull$858) {
result$859 =
org.apache.flink.table.data.binary.BinaryStringDataUtil.substringSQL(field$857,
((int) 1), ((int) 5));
isNull$858 = (result$859 == null);
}
isNull$860 = isNull$854 || isNull$858;
result$861 = false;
if (!isNull$860) {
result$861 = result$855.equals(result$859);
}
return (!result$861); // this is wrong
```
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33171][table planner] Consistent implicit type coercion support for equal and non-equal comparisons for codegen [flink]
fengjiajie commented on PR #23478: URL: https://github.com/apache/flink/pull/23478#issuecomment-1775428180 @flinkbot run azure -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33171][table planner] Consistent implicit type coercion support for equal and non-equal comparisons for codegen [flink]
fengjiajie commented on PR #23478: URL: https://github.com/apache/flink/pull/23478#issuecomment-1775246263 @flinkbot run azure -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33171][table planner] Consistent implicit type coercion support for equal and non-equal comparisons for codegen [flink]
fengjiajie commented on PR #23478: URL: https://github.com/apache/flink/pull/23478#issuecomment-1774955380 @flinkbot run azure -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33171][table planner] Consistent implicit type coercion support for equal and non-equal comparisons for codegen [flink]
LadyForest commented on PR #23478: URL: https://github.com/apache/flink/pull/23478#issuecomment-1774684679 @flinkbot run azure -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33171][table planner] Consistent implicit type coercion support for equal and non-equal comparisons for codegen [flink]
fengjiajie commented on PR #23478: URL: https://github.com/apache/flink/pull/23478#issuecomment-1771947970 > Thanks for the update, looks good to me in general, and I just left some minor comments. Thank you for the suggestion. It has been modified. -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33171][table planner] Consistent implicit type coercion support for equal and non-equal comparisons for codegen [flink]
LadyForest commented on code in PR #23478:
URL: https://github.com/apache/flink/pull/23478#discussion_r1365165868
##
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala:
##
@@ -345,44 +345,83 @@ object ScalarOperatorGens {
}
}
- def generateEquals(
+ private def wrapExpressionIfNonEq(
+ isNonEq: Boolean,
+ equalsExpr: GeneratedExpression,
+ resultType: LogicalType): GeneratedExpression = {
+if (isNonEq) {
+ GeneratedExpression(
+s"(!${equalsExpr.resultTerm})",
+equalsExpr.nullTerm,
+equalsExpr.code,
+resultType)
+} else {
+ equalsExpr
+}
+ }
+
+ private def generateEqualAndNonEqual(
ctx: CodeGeneratorContext,
left: GeneratedExpression,
right: GeneratedExpression,
+ operator: String,
resultType: LogicalType): GeneratedExpression = {
+
checkImplicitConversionValidity(left, right)
+
+val nonEq = operator match {
+ case "==" => false
+ case "!=" => true
+ case _ => throw new CodeGenException(s"Unsupported boolean comparison
'$operator'.")
+}
val canEqual = isInteroperable(left.resultType, right.resultType)
+
if (isCharacterString(left.resultType) &&
isCharacterString(right.resultType)) {
generateOperatorIfNotNull(ctx, resultType, left, right)(
-(leftTerm, rightTerm) => s"$leftTerm.equals($rightTerm)")
+(leftTerm, rightTerm) => s"${if (nonEq) "!" else
""}$leftTerm.equals($rightTerm)")
Review Comment:
Nit
```scala
wrapExpressionIfNonEq(
nonEq,
generateOperatorIfNotNull(ctx, resultType, left, right)(
(leftTerm, rightTerm) => s"$leftTerm.equals($rightTerm)"),
resultType)
```
##
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala:
##
@@ -409,32 +448,44 @@ object ScalarOperatorGens {
// for performance, we cast literal string to literal time.
else if (isTimePoint(left.resultType) &&
isCharacterString(right.resultType)) {
if (right.literal) {
-generateEquals(ctx, left, generateCastLiteral(ctx, right,
left.resultType), resultType)
+generateEqualAndNonEqual(
+ ctx,
+ left,
+ generateCastLiteral(ctx, right, left.resultType),
+ operator,
+ resultType)
} else {
-generateEquals(
+generateEqualAndNonEqual(
ctx,
left,
generateCast(ctx, right, left.resultType, nullOnFailure = true),
+ operator,
resultType)
}
} else if (isTimePoint(right.resultType) &&
isCharacterString(left.resultType)) {
if (left.literal) {
-generateEquals(ctx, generateCastLiteral(ctx, left, right.resultType),
right, resultType)
+generateEqualAndNonEqual(
+ ctx,
+ generateCastLiteral(ctx, left, right.resultType),
+ right,
+ operator,
+ resultType)
} else {
-generateEquals(
+generateEqualAndNonEqual(
ctx,
generateCast(ctx, left, right.resultType, nullOnFailure = true),
right,
+ operator,
resultType)
}
}
// non comparable types
else {
generateOperatorIfNotNull(ctx, resultType, left, right) {
if (isReference(left.resultType)) {
Review Comment:
Nit
```scala
// non comparable types
else {
val (newLeft, newRight) = if (isReference(left.resultType)) {
(left, right)
} else if (isReference(right.resultType)) {
(right, left)
} else {
throw new CodeGenException(
s"Incomparable types: ${left.resultType} and " +
s"${right.resultType}")
}
wrapExpressionIfNonEq(
nonEq,
generateOperatorIfNotNull(ctx, resultType, newLeft, newRight) {
(leftTerm, rightTerm) => s"$leftTerm.equals($rightTerm)"
},
resultType
)
}
```
##
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala:
##
@@ -409,32 +448,44 @@ object ScalarOperatorGens {
// for performance, we cast literal string to literal time.
else if (isTimePoint(left.resultType) &&
isCharacterString(right.resultType)) {
if (right.literal) {
-generateEquals(ctx, left, generateCastLiteral(ctx, right,
left.resultType), resultType)
+generateEqualAndNonEqual(
Review Comment:
Nit: maybe we can simplify the logic here
```scala
else if (
(isTimePoint(left.resultType) && isCharacterString(right.resultType))
|| (isTimePoint(
right.resultType) && isCharacterString(left.resultType))
) {
val (newLeft, newRight) =
Re: [PR] [FLINK-33171][table planner] Consistent implicit type coercion support for equal and non-equal comparisons for codegen [flink]
LadyForest commented on PR #23478: URL: https://github.com/apache/flink/pull/23478#issuecomment-1770498589 > newRelDataType > > @lincoln-lil Thank you for your review. Are the 5 datatypes you mentioned referring to DATE, TIME_WITHOUT_TIME_ZONE, TIMESTAMP_WITHOUT_TIME_ZONE, TIMESTAMP_WITH_TIME_ZONE, and TIMESTAMP_WITH_LOCAL_TIME_ZONE? In the previous test code, there were f15, f21, and f22 corresponding to the first 3 types. I attempted to add the last two, but encountered an exception: > > ``` > > org.apache.flink.table.api.TableException: Type is not supported: TIMESTAMP_WITH_TIME_ZONE > > at org.apache.flink.table.planner.calcite.FlinkTypeFactory.newRelDataType$1(FlinkTypeFactory.scala:152) > > ``` > > > > > > So now I have only added f23, which corresponds to the TIMESTAMP_WITH_LOCAL_TIME_ZONE. If any further modifications are needed, please let me know. Thank you. > > @fengjiajie The 5 types are correct, I checked the code in FlinkTypeFactory and also verified it in sql client, the `TIMESTAMP_WITH_TIME_ZONE` type is unsupported, this can be a separate issue to follow. > > errors in sql client when create a column with TIMESTAMP WITH TIME ZONE: > > ```sql > Flink SQL> CREATE TABLE Bid ( > > bid STRING, > > price BIGINT, > > rowtime TIMESTAMP WITH TIME ZONE, > > -- declare user_action_time as event time attribute and use 5 seconds delayed watermark strategy > > WATERMARK FOR rowtime AS rowtime - INTERVAL '5' SECOND > > ) WITH ( 'connector' = 'datagen'); > [ERROR] Could not execute SQL statement. Reason: > org.apache.flink.sql.parser.impl.ParseException: Encountered "TIME" at line 4, column 26. > Was expecting: > "LOCAL" ... > ``` Thanks for bringing up this issue. I think fixing this issue requires both support from the syntax and type factory. -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33171][table planner] Consistent implicit type coercion support for equal and non-equal comparisons for codegen [flink]
LadyForest commented on PR #23478:
URL: https://github.com/apache/flink/pull/23478#issuecomment-1767854974
> > 1. When constructing "non-comparable types," there is no problem with
`testSqlApi("NULL = f30", "NULL")`, but `testSqlApi("NULL = f30", "NULL")`
throws an exception:
> >`Caused by: org.apache.calcite.sql.validate.SqlValidatorException:
Cannot apply '=' to arguments of type ' =
'.
Supported form(s): ' = '`
> > 2. When constructing "non-comparable types," I haven't figured out how
to cover the branch:
> >`} else if (isReference(right.resultType)) {` because the preceding
> >`if (isReference(left.resultType)) {` always takes precedence.
> > 3. I have written an SQL test for MULTISET in the MiscITCase. I'm not
sure if this is appropriate.
>
> @fengjiajie IIUC, we should add an explicit type `cast` to `null` so that
the field type can be deterministic, e.g., 'cast(null as bigint)' to construct
a empty bigint column. For the `multiset` type, I'm fine with the current
`MiscITCase` or the `CalcItCase`. Btw, during the community review process, we
generally recommend using the append commits, which makes it easier for the
reviewer to track changes during the process and squash them when merging at
the end.
>
> cc @LadyForest If you have time to help check it again that would be great
:)
I'm sorry for not getting back to you sooner. I'll take a look as soon as
possible.
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33171][table planner] Consistent implicit type coercion support for equal and non-equal comparisons for codegen [flink]
lincoln-lil commented on PR #23478:
URL: https://github.com/apache/flink/pull/23478#issuecomment-1766456638
> 1. When constructing "non-comparable types," there is no problem with
`testSqlApi("NULL = f30", "NULL")`, but `testSqlApi("NULL = f30", "NULL")`
throws an exception:
>`Caused by: org.apache.calcite.sql.validate.SqlValidatorException:
Cannot apply '=' to arguments of type ' =
'.
Supported form(s): ' = '`
> 2. When constructing "non-comparable types," I haven't figured out how to
cover the branch:
>`} else if (isReference(right.resultType)) {` because the preceding
>`if (isReference(left.resultType)) {` always takes precedence.
> 3. I have written an SQL test for MULTISET in the MiscITCase. I'm not sure
if this is appropriate.
@fengjiajie IIUC, we should add an explicit type `cast` to `null` so that
the field type can be deterministic, e.g., 'cast(null as bigint)' to construct
a empty bigint column. For the `multiset` type, I'm fine with the current
`MiscITCase` or the `CalcItCase`.
Btw, during the community review process, we generally recommend using the
append commits, which makes it easier for the reviewer to track changes during
the process and squash them when merging at the end.
cc @LadyForest If you have time to help check it again that would be great :)
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33171][table planner] Consistent implicit type coercion support for equal and non-equal comparisons for codegen [flink]
fengjiajie commented on PR #23478:
URL: https://github.com/apache/flink/pull/23478#issuecomment-1764364941
1. When constructing "non-comparable types," there is no problem with
`testSqlApi("NULL = f30", "NULL")`, but `testSqlApi("NULL = f30", "NULL")`
throws an exception:
```Caused by: org.apache.calcite.sql.validate.SqlValidatorException:
Cannot apply '=' to arguments of type ' =
'.
Supported form(s): ' = '```
2. When constructing "non-comparable types," I haven't figured out how to
cover the branch:
```} else if (isReference(right.resultType)) {``` because the preceding
```if (isReference(left.resultType)) {``` always takes precedence.
3. I have written an SQL test for MULTISET in the MiscITCase. I'm not sure
if this is appropriate.
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
