You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "snuyanzin (via GitHub)" <gi...@apache.org> on 2026/10/08 21:08:30 UTC
[PR] [FLINK-40965][table] Treat `IF` as conditional [flink]
snuyanzin opened a new pull request, #29438:
URL: https://github.com/apache/flink/pull/29438
## What is the purpose of the change
`IF` should be treated as conditional
## Brief change log
*(for example:)*
- *The TaskInfo is stored in the blob store on job creation time as a persistent artifact*
- *Deployments RPC transmits only the blob storage reference*
- *TaskManagers retrieve the TaskInfo from the blob cache*
## Verifying this change
short circuit test provided
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (yes / no)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (yes / no)
- The serializers: (yes / no / don't know)
- The runtime per-record code paths (performance sensitive): (yes / no / don't know)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / no / don't know)
- The S3 file system connector: (yes / no / don't know)
## Documentation
- Does this pull request introduce a new feature? (yes / no)
- If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
---
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR, please
change the checkbox below to `[X]` and replace the placeholder in the "Generated-by"
line with the tool name and version. Otherwise remove the "Generated-by" line.
See the ASF Generative Tooling Guidance for details:
https://www.apache.org/legal/generative-tooling.html
You are responsible for the quality and correctness of every change in this PR
regardless of the tooling used. Low-effort AI-generated PRs will be closed. See
AGENTS.md for the full guidance.
-->
- [ ] Yes (please specify the tool below)
Generated-by: [Tool Name and Version]
--
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-40965][table] Treat `IF` as conditional [flink]
Posted by "AHeise (via GitHub)" <gi...@apache.org>.
AHeise commented on code in PR #29438:
URL: https://github.com/apache/flink/pull/29438#discussion_r4228079352
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/ExprCodeGenerator.scala:
##########
@@ -565,10 +565,13 @@ class ExprCodeGenerator(
* - `CASE(when_1, then_1, when_2, then_2, ..., else)`: only `when_1` is unconditional.
* - `AND(a_0, a_1, ..., a_n)` / `OR(...)`: only `a_0` is unconditional; subsequent operands are
* short-circuited by the operator semantics and the codegen.
+ * - `IF(cond, then, else)`: only `cond` is unconditional; `IfCallGen` guards `then`/`else`.
*/
private def conditionalOperandIndices(call: RexCall): Set[Int] = call.getKind match {
case SqlKind.CASE | SqlKind.AND | SqlKind.OR | SqlKind.COALESCE =>
Review Comment:
`COALESCE` from a compiled plan still reaches codegen as a `BridgingSqlFunction` (`$COALESCE$1`, see `calc-coalesce.json`), which always has `SqlKind.OTHER_FUNCTION`. So this case doesn't match and the later arguments get hoisted, although `generateCoalesce` guards them. Same bug as `IF`. Could we cover it here as well (match `BridgingSqlFunction` with `BuiltInFunctionDefinitions.COALESCE`), plus a restore test where a later argument would fail?
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/ExprCodeGenerator.scala:
##########
@@ -565,10 +565,13 @@ class ExprCodeGenerator(
* - `CASE(when_1, then_1, when_2, then_2, ..., else)`: only `when_1` is unconditional.
* - `AND(a_0, a_1, ..., a_n)` / `OR(...)`: only `a_0` is unconditional; subsequent operands are
* short-circuited by the operator semantics and the codegen.
+ * - `IF(cond, then, else)`: only `cond` is unconditional; `IfCallGen` guards `then`/`else`.
Review Comment:
Nit: `COALESCE` is handled below but missing from this list. Could you add it while you're here?
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/ExprCodeGenerator.scala:
##########
@@ -565,10 +565,13 @@ class ExprCodeGenerator(
* - `CASE(when_1, then_1, when_2, then_2, ..., else)`: only `when_1` is unconditional.
* - `AND(a_0, a_1, ..., a_n)` / `OR(...)`: only `a_0` is unconditional; subsequent operands are
* short-circuited by the operator semantics and the codegen.
+ * - `IF(cond, then, else)`: only `cond` is unconditional; `IfCallGen` guards `then`/`else`.
*/
private def conditionalOperandIndices(call: RexCall): Set[Int] = call.getKind match {
case SqlKind.CASE | SqlKind.AND | SqlKind.OR | SqlKind.COALESCE =>
(1 until call.getOperands.size).toSet
+ case SqlKind.OTHER_FUNCTION if call.getOperator == IF =>
Review Comment:
Nit: `IF` is a singleton, so the kind check is redundant: `case _ if call.getOperator eq IF =>`.
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/runtime/stream/sql/FunctionITCase.java:
##########
@@ -1346,6 +1346,27 @@ void testCalcCaseGuardShortCircuit() {
assertThat(actual).containsExactly(Row.of(10), Row.of(30));
}
+ /** Pins the IF guard interaction with the RexLocalRef cache (IF is not scoped like CASE). */
Review Comment:
"IF is not scoped like CASE" reads like the opposite of what this PR does. Maybe mirror the CASE test's Javadoc: without scoping, the ELSE-branch cast was hoisted to the method top and threw on `""`; with scoping, it stays inside the branch.
--
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-40965][table] Treat `IF` as conditional [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin closed pull request #29438: [FLINK-40965][table] Treat `IF` as conditional
URL: https://github.com/apache/flink/pull/29438
--
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-40965][table] Treat `IF` as conditional [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #29438:
URL: https://github.com/apache/flink/pull/29438#discussion_r4228828477
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/ExprCodeGenerator.scala:
##########
@@ -565,10 +565,13 @@ class ExprCodeGenerator(
* - `CASE(when_1, then_1, when_2, then_2, ..., else)`: only `when_1` is unconditional.
* - `AND(a_0, a_1, ..., a_n)` / `OR(...)`: only `a_0` is unconditional; subsequent operands are
* short-circuited by the operator semantics and the codegen.
+ * - `IF(cond, then, else)`: only `cond` is unconditional; `IfCallGen` guards `then`/`else`.
Review Comment:
added
--
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-40965][table] Treat `IF` as conditional [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #29438:
URL: https://github.com/apache/flink/pull/29438#issuecomment-6069265679
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "f91bef04987721f6bf92df55fb2589f21ba1bcec",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "f91bef04987721f6bf92df55fb2589f21ba1bcec",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* f91bef04987721f6bf92df55fb2589f21ba1bcec 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-40965][table] Treat `IF` as conditional [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #29438:
URL: https://github.com/apache/flink/pull/29438#discussion_r4228836413
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/ExprCodeGenerator.scala:
##########
@@ -565,10 +565,13 @@ class ExprCodeGenerator(
* - `CASE(when_1, then_1, when_2, then_2, ..., else)`: only `when_1` is unconditional.
* - `AND(a_0, a_1, ..., a_n)` / `OR(...)`: only `a_0` is unconditional; subsequent operands are
* short-circuited by the operator semantics and the codegen.
+ * - `IF(cond, then, else)`: only `cond` is unconditional; `IfCallGen` guards `then`/`else`.
*/
private def conditionalOperandIndices(call: RexCall): Set[Int] = call.getKind match {
case SqlKind.CASE | SqlKind.AND | SqlKind.OR | SqlKind.COALESCE =>
Review Comment:
thanks for flagging
since it might require restore tests i would do it separately (first try to repro it locally)
--
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