twalthr commented on code in PR #29830:
URL: https://github.com/apache/flink/pull/29830#discussion_r4513573631
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/ExprCodeGenerator.scala:
##########
@@ -1053,6 +1053,35 @@ class ExprCodeGenerator(
}.toArray
}
+ /**
+ * A local ref generated in a guarded scope is generated again in every
other scope that uses it,
+ * each time under new terms. All copies write to the same variables, so
code keyed on the result
+ * term (e.g. the shared JSON parse) sees one input.
Review Comment:
can you add a SQL example that helps the reader what special case is handled
here?
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/codegen/XmlReadReuseTest.java:
##########
@@ -105,6 +117,32 @@ void testCallsOnDifferentInputsReadEachOnce() {
assertThat(countReads(sql)).isEqualTo(2);
}
+ static Stream<Arguments> computedInputInCaseBranches() {
+ return Stream.of(
+ Arguments.of(
+ "CAST(PARSE_XML(x)['book']['title'] AS STRING)",
+ "CAST(PARSE_XML(x, TRUE)['book']['title'][1] AS
STRING)"),
+ Arguments.of(
+ "CAST(PARSE_XML(x)['book']['title'] AS STRING)",
+ "CAST(TRY_PARSE_XML(x, TRUE)['book']['title'][1] AS
STRING)"));
+ }
+
+ @ParameterizedTest(name = "{0} | {1}")
+ @MethodSource("computedInputInCaseBranches")
+ void testComputedInputInDifferentCaseBranchesReadsOnce(
+ final String firstBranch, final String secondBranch) {
+ // x = TRIM(xml) is recomputed in each branch, under a new term each
time
+ final String sql =
+ ("SELECT CASE WHEN other_xml = '<a>1</a>' THEN "
+ + firstBranch
+ + " WHEN other_xml = '<a>2</a>' THEN "
+ + secondBranch
+ + " ELSE NULL END FROM xml_src")
+ .replace("(x", "(TRIM(xml)");
Review Comment:
```suggestion
.replace("???", "TRIM(xml)");
```
nit: use better template variable that avoid replace `(`with another `(`
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/ExprCodeGenerator.scala:
##########
@@ -1053,6 +1053,35 @@ class ExprCodeGenerator(
}.toArray
}
+ /**
+ * A local ref generated in a guarded scope is generated again in every
other scope that uses it,
+ * each time under new terms. All copies write to the same variables, so
code keyed on the result
+ * term (e.g. the shared JSON parse) sees one input.
+ */
+ private def withSharedTerms(idx: Int, expr: GeneratedExpression):
GeneratedExpression = {
+ if (
+ expr.code.trim.isEmpty ||
Review Comment:
```suggestion
expr.code.isEmpty ||
```
this is not user code, it comes from Flink, trim should not be neccessary.
if it is, there is a problem upstream, no?
--
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]
