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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
