You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "xuyangzhong (via GitHub)" <gi...@apache.org> on 2024/08/21 09:51:20 UTC
[PR] [FLINK-35827][table-planner] Correcting equality comparisons between rowType fields and constants [flink]
xuyangzhong opened a new pull request, #25229:
URL: https://github.com/apache/flink/pull/25229
## What is the purpose of the change
The first commit tries to remove the redundant cast in row type with PEEK_FIELDS_NO_EXPAND.
The second commit tries to fix the wrong equality comparison result between rowType fields and constants.
## Brief change log
*(for example:)*
- *When trying to find the least restrictive StructuredType between row types with PEEK_FIELDS_NO_EXPAND, it should not return row types with FULLY_QUALIFIED.*
- *The classes with two RowData may be different. When we check the equality of row types, we should check the type of RowData first, just like checking array, map and other types.*
- *Add tests to verify these two commits.*
## Verifying this change
Tests are added to verify these two commits.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: no
- The serializers: no
- The runtime per-record code paths (performance sensitive): no
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35827][table-planner] Correcting equality comparisons between rowType fields and constants [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #25229:
URL: https://github.com/apache/flink/pull/25229#issuecomment-2301643767
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "9e2c29d8344fcc133691f4950867664dcaaa7db3",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "9e2c29d8344fcc133691f4950867664dcaaa7db3",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 9e2c29d8344fcc133691f4950867664dcaaa7db3 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35827][table-planner] Correcting equality comparisons between rowType fields and constants [flink]
Posted by "lincoln-lil (via GitHub)" <gi...@apache.org>.
lincoln-lil commented on PR #25229:
URL: https://github.com/apache/flink/pull/25229#issuecomment-2325739689
> If commit-1 is missing, the bug fixed by commit-2 will only appear in the Table API and not in the SQL API. This is because the SQL API will always cast the PEEK_FIELDS_NO_EXPAND RowType to FULLY_QUALIFIED by Calcite, resulting in no issues since every RowType data will be in BinaryRowData. However, it’s important to note that in Flink, the StructKind of RowType should consistently be the PEEK_FIELDS_NO_EXPAND throughout the entire process.
@xuyangzhong thanks for fixing this! The 1st commit seems not a necessary change for this fix, if there exists a concrete case in sql area, we can fix it separately, WDYT?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35827][table-planner] Correcting equality comparisons between rowType fields and constants [flink]
Posted by "xuyangzhong (via GitHub)" <gi...@apache.org>.
xuyangzhong commented on PR #25229:
URL: https://github.com/apache/flink/pull/25229#issuecomment-2301645610
If commit-1 is missing, the bug fixed by commit-2 will only appear in the Table API and not in the SQL API.
This is because the SQL API will always cast the PEEK_FIELDS_NO_EXPAND RowType to FULLY_QUALIFIED by Calcite, resulting in no issues since every RowType data will be in BinaryRowData.
However, it’s important to note that in Flink, the properties of RowType fields should consistently reflect the PEEK_FIELDS_NO_EXPAND attribute throughout the entire process.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35827][table-planner] Correcting equality comparisons between rowType fields and constants [flink]
Posted by "lincoln-lil (via GitHub)" <gi...@apache.org>.
lincoln-lil commented on code in PR #25229:
URL: https://github.com/apache/flink/pull/25229#discussion_r1746719954
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala:
##########
@@ -1818,6 +1822,27 @@ object ScalarOperatorGens {
(stmt, resultTerm)
}
+ private def generateRowComparison(
+ ctx: CodeGeneratorContext,
+ left: GeneratedExpression,
+ right: GeneratedExpression,
+ resultType: LogicalType): GeneratedExpression = {
+ generateCallWithStmtIfArgsNotNull(ctx, resultType, Seq(left, right)) {
+ args =>
+ val leftTerm = args.head
+ val rightTerm = args(1)
+
+ val resultTerm = newName(ctx, "compareResult")
Review Comment:
`resultTerm` is unused.
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/EqualiserCodeGenerator.scala:
##########
@@ -174,3 +186,32 @@ class EqualiserCodeGenerator(fieldTypes: Array[LogicalType], classLoader: ClassL
case _ => false
}
}
+
+object EqualiserCodeGenerator {
+
+ def generateRecordEqualiserCode(
+ ctx: CodeGeneratorContext,
+ leftFieldType: LogicalType,
+ rightFieldType: LogicalType,
+ leftFieldTerm: String,
+ rightFieldTerm: String,
+ generatedEqualiserName: String) = {
Review Comment:
nit: Add return type explicitly `: (String, String)`
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35827][table-planner] Correcting equality comparisons between rowType fields and constants [flink]
Posted by "xuyangzhong (via GitHub)" <gi...@apache.org>.
xuyangzhong commented on PR #25229:
URL: https://github.com/apache/flink/pull/25229#issuecomment-2330686539
@lincoln-lil Agree with you. I have updated this pr and removed the first commit.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-35827][table-planner] Correcting equality comparisons between rowType fields and constants [flink]
Posted by "lincoln-lil (via GitHub)" <gi...@apache.org>.
lincoln-lil merged PR #25229:
URL: https://github.com/apache/flink/pull/25229
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org