twalthr commented on code in PR #29825:
URL: https://github.com/apache/flink/pull/29825#discussion_r4512842755
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala:
##########
@@ -1078,49 +1078,68 @@ object ScalarOperatorGens {
def generateIfElse(
ctx: CodeGeneratorContext,
operands: Seq[GeneratedExpression],
- resultType: LogicalType,
- i: Int = 0): GeneratedExpression = {
- // else part
- if (i == operands.size - 1) {
- generateCast(ctx, operands(i), resultType, nullOnFailure = true)
- } else {
- // check that the condition is boolean
- // we do not check for null instead we use the default value
- // thus null is false
- requireBoolean(operands(i))
- val condition = operands(i)
- val trueAction = generateCast(ctx, operands(i + 1), resultType,
nullOnFailure = true)
- val falseAction = generateIfElse(ctx, operands, resultType, i + 2)
+ resultType: LogicalType): GeneratedExpression = {
+ if (operands.size == 1) {
+ return generateCast(ctx, operands.head, resultType, nullOnFailure = true)
+ }
+ val Seq(resultTerm, nullTerm) = newNames(ctx, "result", "isNull")
+ val resultTypeTerm = primitiveTypeTermForType(resultType)
+ val defaultValue = primitiveDefaultValue(resultType)
- val Seq(resultTerm, nullTerm) = newNames(ctx, "result", "isNull")
- val resultTypeTerm = primitiveTypeTermForType(resultType)
- val defaultValue = primitiveDefaultValue(resultType)
+ def assign(action: GeneratedExpression): String =
+ s"""
+ |${action.code}
+ |$nullTerm = ${action.nullTerm};
+ |if (!$nullTerm) {
+ | $resultTerm = ${action.resultTerm};
+ |}
+ |""".stripMargin
- val operatorCode =
- s"""
- |${condition.code}
- |$resultTypeTerm $resultTerm = $defaultValue;
- |boolean $nullTerm;
- |if (${condition.resultTerm}) {
- | ${trueAction.code}
- | $nullTerm = ${trueAction.nullTerm};
- | if (!$nullTerm) {
- | $resultTerm = ${trueAction.resultTerm};
- | }
- |}
- |else {
- | ${falseAction.code}
- | $nullTerm = ${falseAction.nullTerm};
- | if (!$nullTerm) {
- | $resultTerm = ${falseAction.resultTerm};
- | }
- |}
- |""".stripMargin.trim
+ // a null condition is false via its default value
+ val whens = (0 until operands.size - 1 by 2).map {
+ j =>
+ requireBoolean(operands(j))
+ (operands(j), generateCast(ctx, operands(j + 1), resultType,
nullOnFailure = true))
+ }
+ val elseAction = generateCast(ctx, operands.last, resultType,
nullOnFailure = true)
- GeneratedExpression(resultTerm, nullTerm, operatorCode, resultType)
+ // flat guarded blocks instead of nested else keep the splitter output flat
+ val doneTerm = newName(ctx, "done")
+ val branches = whens.zipWithIndex.map {
+ case ((condition, trueAction), i) =>
+ val branch =
+ s"""
+ |${condition.code}
+ |if (${condition.resultTerm}) {
+ | ${assign(trueAction)}
+ | $doneTerm = true;
+ |}
+ |""".stripMargin
+ // the first branch of a chunk is guarded by its chunk
+ if (i % CASE_CHUNK_WHENS == 0) branch else s"if (!$doneTerm)
{\n$branch\n}"
Review Comment:
AI suggests:
```
Guards: the "is this the first branch of a chunk" check (i %
CASE_CHUNK_WHENS == 0) and the chunk-level guard express the same idea in two
places. Grouping first, and then guarding everything except element 0 of each
chunk, would be simpler.
```
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala:
##########
@@ -1078,49 +1078,68 @@ object ScalarOperatorGens {
def generateIfElse(
ctx: CodeGeneratorContext,
operands: Seq[GeneratedExpression],
- resultType: LogicalType,
- i: Int = 0): GeneratedExpression = {
- // else part
- if (i == operands.size - 1) {
- generateCast(ctx, operands(i), resultType, nullOnFailure = true)
- } else {
- // check that the condition is boolean
- // we do not check for null instead we use the default value
- // thus null is false
- requireBoolean(operands(i))
- val condition = operands(i)
- val trueAction = generateCast(ctx, operands(i + 1), resultType,
nullOnFailure = true)
- val falseAction = generateIfElse(ctx, operands, resultType, i + 2)
+ resultType: LogicalType): GeneratedExpression = {
+ if (operands.size == 1) {
+ return generateCast(ctx, operands.head, resultType, nullOnFailure = true)
+ }
+ val Seq(resultTerm, nullTerm) = newNames(ctx, "result", "isNull")
+ val resultTypeTerm = primitiveTypeTermForType(resultType)
+ val defaultValue = primitiveDefaultValue(resultType)
- val Seq(resultTerm, nullTerm) = newNames(ctx, "result", "isNull")
- val resultTypeTerm = primitiveTypeTermForType(resultType)
- val defaultValue = primitiveDefaultValue(resultType)
+ def assign(action: GeneratedExpression): String =
+ s"""
+ |${action.code}
+ |$nullTerm = ${action.nullTerm};
+ |if (!$nullTerm) {
+ | $resultTerm = ${action.resultTerm};
+ |}
+ |""".stripMargin
- val operatorCode =
- s"""
- |${condition.code}
- |$resultTypeTerm $resultTerm = $defaultValue;
- |boolean $nullTerm;
- |if (${condition.resultTerm}) {
- | ${trueAction.code}
- | $nullTerm = ${trueAction.nullTerm};
- | if (!$nullTerm) {
- | $resultTerm = ${trueAction.resultTerm};
- | }
- |}
- |else {
- | ${falseAction.code}
- | $nullTerm = ${falseAction.nullTerm};
- | if (!$nullTerm) {
- | $resultTerm = ${falseAction.resultTerm};
- | }
- |}
- |""".stripMargin.trim
+ // a null condition is false via its default value
+ val whens = (0 until operands.size - 1 by 2).map {
+ j =>
+ requireBoolean(operands(j))
+ (operands(j), generateCast(ctx, operands(j + 1), resultType,
nullOnFailure = true))
+ }
+ val elseAction = generateCast(ctx, operands.last, resultType,
nullOnFailure = true)
- GeneratedExpression(resultTerm, nullTerm, operatorCode, resultType)
+ // flat guarded blocks instead of nested else keep the splitter output flat
+ val doneTerm = newName(ctx, "done")
+ val branches = whens.zipWithIndex.map {
+ case ((condition, trueAction), i) =>
+ val branch =
+ s"""
+ |${condition.code}
+ |if (${condition.resultTerm}) {
+ | ${assign(trueAction)}
+ | $doneTerm = true;
+ |}
+ |""".stripMargin
+ // the first branch of a chunk is guarded by its chunk
+ if (i % CASE_CHUNK_WHENS == 0) branch else s"if (!$doneTerm)
{\n$branch\n}"
}
+ // one guard per chunk, so after a match the remaining chunks cost one
check each even when
+ // the code splitter moves their branches into methods
+ val chunks = branches.grouped(CASE_CHUNK_WHENS).zipWithIndex.map {
+ case (chunk, 0) => chunk.mkString("\n")
+ case (chunk, _) => s"if (!$doneTerm) {\n${chunk.mkString("\n")}\n}"
+ }
+ val code =
+ s"""
+ |$resultTypeTerm $resultTerm = $defaultValue;
+ |boolean $nullTerm = true;
+ |boolean $doneTerm = false;
+ |${chunks.mkString("\n")}
+ |if (!$doneTerm) {
+ | ${assign(elseAction)}
+ |}
+ |""".stripMargin.trim
+ GeneratedExpression(resultTerm, nullTerm, code, resultType)
}
+ // Number of WHENs of a CASE behind one shared guard.
+ private val CASE_CHUNK_WHENS = 10
Review Comment:
Explain why you chose this constant.
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala:
##########
@@ -1078,49 +1078,68 @@ object ScalarOperatorGens {
def generateIfElse(
ctx: CodeGeneratorContext,
operands: Seq[GeneratedExpression],
- resultType: LogicalType,
- i: Int = 0): GeneratedExpression = {
- // else part
- if (i == operands.size - 1) {
- generateCast(ctx, operands(i), resultType, nullOnFailure = true)
- } else {
- // check that the condition is boolean
- // we do not check for null instead we use the default value
- // thus null is false
- requireBoolean(operands(i))
- val condition = operands(i)
- val trueAction = generateCast(ctx, operands(i + 1), resultType,
nullOnFailure = true)
- val falseAction = generateIfElse(ctx, operands, resultType, i + 2)
+ resultType: LogicalType): GeneratedExpression = {
+ if (operands.size == 1) {
+ return generateCast(ctx, operands.head, resultType, nullOnFailure = true)
+ }
+ val Seq(resultTerm, nullTerm) = newNames(ctx, "result", "isNull")
+ val resultTypeTerm = primitiveTypeTermForType(resultType)
+ val defaultValue = primitiveDefaultValue(resultType)
- val Seq(resultTerm, nullTerm) = newNames(ctx, "result", "isNull")
- val resultTypeTerm = primitiveTypeTermForType(resultType)
- val defaultValue = primitiveDefaultValue(resultType)
+ def assign(action: GeneratedExpression): String =
+ s"""
+ |${action.code}
+ |$nullTerm = ${action.nullTerm};
+ |if (!$nullTerm) {
+ | $resultTerm = ${action.resultTerm};
+ |}
+ |""".stripMargin
- val operatorCode =
- s"""
- |${condition.code}
- |$resultTypeTerm $resultTerm = $defaultValue;
- |boolean $nullTerm;
- |if (${condition.resultTerm}) {
- | ${trueAction.code}
- | $nullTerm = ${trueAction.nullTerm};
- | if (!$nullTerm) {
- | $resultTerm = ${trueAction.resultTerm};
- | }
- |}
- |else {
- | ${falseAction.code}
- | $nullTerm = ${falseAction.nullTerm};
- | if (!$nullTerm) {
- | $resultTerm = ${falseAction.resultTerm};
- | }
- |}
- |""".stripMargin.trim
+ // a null condition is false via its default value
+ val whens = (0 until operands.size - 1 by 2).map {
Review Comment:
AI flagged this to me:
```
Even operand count. generateIfElse doesn't check that it has an odd number
of operands. With an even count, operands.last would be used as both the last
THEN and the ELSE, producing wrong code without any error (the old code failed
with an index error instead). Calcite never produces this, but a
require(operands.size % 2 == 1) would be cheap.
```
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/CaseFunctionsITCase.java:
##########
@@ -37,6 +37,22 @@ Stream<TestSetSpec> getTestSetSpecs() {
.collect(Collectors.joining(" "))
+ "else 9999 end";
+ // overlapping conditions: every WHEN from f0 on is true
+ String overlappingCaseStatement =
Review Comment:
Feedback for testing:
```
- No error test on a branch protected only by the chunk guard. In
failingLaterCaseStatement, the dividing WHEN is at position 25, so it is
protected by both its own guard and its chunk's guard. The first WHEN of a
chunk (position 10 or 20) is protected only by the chunk guard. Moving the
failing WHEN to position 20 would test exactly that. The overlapping-conditions
test does cover the chunk guard for overwrites, because its match at 110 is a
chunk's first WHEN.
- No NULL result from a matched WHEN in a later chunk. For example: ... WHEN
f0 = 110 THEN CAST(NULL AS INT) ... ELSE 1, expecting NULL.
- No test for a CASE without ELSE and no match (which must return NULL).
This matters now that isNull starts as true.
- No long CASE with a nullable condition (a nullable BOOLEAN column) after
the first chunk.
- No long CASE returning reference types (STRING, DECIMAL, TIMESTAMP,
ROW/ARRAY). Only a short CHAR case is tested.
- No nested long CASE (a CASE inside a WHEN or THEN of another), which would
check that the done flags stay independent once the code is split.
- No tests at the chunk-size boundaries (exactly 10, 11 and 20 WHENs).
- No CodeSplitTest for CASE. For example, about 1000 WHENs that only compile
with splitting, which would protect against the code becoming nested again.
- Some one-line result types are untested: SMALLINT, FLOAT, and
DATE/TIME/INTERVAL, which are stored as plain numbers.
- Misleading test comment. The "operands that are never null" comment covers
f1 + 1 and f1 * 5, but f1 is declared as nullable INT(). Only the literal 1 is
never null, so the "every operand is never null" path (nullCheck == NEVER_NULL)
is never reached there. f5 = FALSE might reach it, but I suspect (without
checking) that Calcite rewrites it to NOT f5, so it may never reach the
comparison code. A two-column comparison of NOT NULL columns would test it
reliably.
- Outdated comment. In CaseFunctionsITCase, the comment "produces deeply
nested if else statements" is no longer true.
```
--
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]
