wenshao commented on code in PR #28836: URL: https://github.com/apache/flink/pull/28836#discussion_r3696464294
########## flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/calls/JsonParseReuse.scala: ########## @@ -0,0 +1,83 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.flink.table.planner.codegen.calls + +import org.apache.flink.table.planner.codegen.{CodeGeneratorContext, CodeGenUtils, GeneratedExpression} +import org.apache.flink.table.planner.codegen.CodeGenUtils.qualifyMethod +import org.apache.flink.table.runtime.functions.SqlJsonUtils + +/** + * Shares the parsed JSON input between the JSON function calls of a single generated expression, + * see [[JsonValueCallGen]] and [[JsonQueryCallGen]]. + * + * The input is parsed at most once per record, by whichever call runs first, and the result is held + * in a member variable that the following calls on the same input read instead of parsing again. + */ +object JsonParseReuse { + + /** + * Returns the expression holding the parsed input. The caller generates its own call with the + * original operands and reads the parsed input from the returned expression. + * + * The parse is lazy: the result term is a call to a member method that parses on its first + * invocation for the record. `generateCallWithStmtIfArgsNotNull` places a call under a guard that + * requires *all* of its arguments to be non-null, so no single call can be made the owner of the + * parse - in + * {{{ + * SELECT JSON_VALUE(v, CAST(NULL AS STRING)), JSON_QUERY(v, '$.a') + * }}} + * the NULL path makes the first call short-circuit while the second one still needs the parse. + * Conversely, when no call runs, no parse happens at all. + */ + def parseSharedInput( + ctx: CodeGeneratorContext, + operands: Seq[GeneratedExpression]): GeneratedExpression = { + val input = operands.head + val inputTerm = s"${input.resultTerm}.toString()" + + ctx.getReusableInputUnboxingExprs(inputTerm, Int.MinValue) match { + case Some(expr) => expr + case None => + val varName = CodeGenUtils.newName(ctx, "jsonParsed") + val lastInputName = CodeGenUtils.newName(ctx, "jsonParsedInput") + val methodName = CodeGenUtils.newName(ctx, "parseJson") + val typeName = classOf[SqlJsonUtils.JsonValueContext].getName + val inputType = CodeGenUtils.BINARY_STRING + ctx.addReusableMember(s"$typeName $varName;") + ctx.addReusableMember(s"$inputType $lastInputName;") + + // keyed on the immutable input so it re-parses on change; no per-record reset needed Review Comment: **[Suggestion]** The comment claims the cache is safe because the input is "immutable," but `BinaryStringData` is technically mutable (it extends `LazyBinaryFormat` which has a public `setJavaObject` setter and package-private mutable fields). The actual safety invariant is narrower: `BinaryRowData.getString()` creates a **new** `BinaryStringData` object per call (via `fromAddress` → `new BinaryStringData(...)`), so reference identity changes per record — Concrete cost: a future maintainer optimizing row unboxing may read "immutable" and conclude the cache is safe regardless of reference identity, or may "fix" `!=` to `!in.equals()`, replacing the O(1) identity check with an O(n) byte comparison on every JSON call site. ```suggestion // Reference-equality cache: BinaryRowData.getString() creates a fresh // BinaryStringData per call, so a new record always brings a new reference. // Do NOT switch to .equals() — the O(1) identity check is the point. ``` _— qwen3.8-max-preview via Qwen Code /review (v0.21.2)_ -- 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]
