gemini-code-assist[bot] commented on code in PR #38951:
URL: https://github.com/apache/beam/pull/38951#discussion_r3414039992
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/CalciteQueryPlanner.java:
##########
@@ -203,13 +272,66 @@ public BeamRelNode convertToBeamRel(String sqlStatement,
QueryParameters queryPa
relNode,
new ParameterBinder(root.rel.getCluster().getRexBuilder(),
queryParameters));
}
- LOG.info("SQLPlan>\n{}", BeamSqlRelUtils.explainLazily(root.rel));
- RelTraitSet desiredTraits =
- relNode
- .getTraitSet()
- .replace(BeamLogicalConvention.INSTANCE)
- .replace(root.collation)
- .simplify();
+ return convertToBeamRel(relNode, root.collation);
+ } catch (RelConversionException | CannotPlanException e) {
+ throw new SqlConversionException(
+ String.format("Unable to convert query %s", sqlStatement), e);
+ } catch (SqlParseException | ValidationException e) {
+ throw new ParseException(String.format("Unable to parse query %s",
sqlStatement), e);
+ }
Review Comment:
If an exception is thrown during parsing or validation (e.g.,
`SqlParseException` or `RelConversionException`), the method exits before
calling the overloaded `convertToBeamRel` method which contains the `finally {
planner.close(); }` block. This leaves the `planner` unclosed, potentially
leaking resources or retaining stale state. We should ensure `planner.close()`
is called in the catch blocks.
```java
} catch (RelConversionException | CannotPlanException e) {
planner.close();
throw new SqlConversionException(
String.format("Unable to convert query %s", sqlStatement), e);
} catch (SqlParseException | ValidationException e) {
planner.close();
throw new ParseException(String.format("Unable to parse query %s",
sqlStatement), e);
}
```
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/CalciteQueryPlanner.java:
##########
@@ -203,13 +272,66 @@ public BeamRelNode convertToBeamRel(String sqlStatement,
QueryParameters queryPa
relNode,
new ParameterBinder(root.rel.getCluster().getRexBuilder(),
queryParameters));
}
- LOG.info("SQLPlan>\n{}", BeamSqlRelUtils.explainLazily(root.rel));
- RelTraitSet desiredTraits =
- relNode
- .getTraitSet()
- .replace(BeamLogicalConvention.INSTANCE)
- .replace(root.collation)
- .simplify();
+ return convertToBeamRel(relNode, root.collation);
+ } catch (RelConversionException | CannotPlanException e) {
+ throw new SqlConversionException(
+ String.format("Unable to convert query %s", sqlStatement), e);
+ } catch (SqlParseException | ValidationException e) {
+ throw new ParseException(String.format("Unable to parse query %s",
sqlStatement), e);
+ }
+ }
+
+ private static RelNode bindParameters(RelNode rel, RexShuttle binder) {
+ RelNode newRel = rel.accept(binder);
+ java.util.List<RelNode> newInputs = new java.util.ArrayList<>();
+ for (RelNode input : newRel.getInputs()) {
+ newInputs.add(bindParameters(input, binder));
+ }
+ return newRel.copy(newRel.getTraitSet(), newInputs);
+ }
Review Comment:
The current implementation of `bindParameters` recursively copies every
single `RelNode` in the tree using `newRel.copy(...)`, even if no parameters
were bound in that subtree. For large query plans, this can be highly
inefficient. We can optimize this by checking if any changes actually occurred
(`newRel != rel` or any of the inputs changed) and only copying when necessary.
```java
private static RelNode bindParameters(RelNode rel, RexShuttle binder) {
RelNode newRel = rel.accept(binder);
java.util.List<RelNode> inputs = newRel.getInputs();
java.util.List<RelNode> newInputs = new
java.util.ArrayList<>(inputs.size());
boolean changed = newRel != rel;
for (RelNode input : inputs) {
RelNode newInput = bindParameters(input, binder);
newInputs.add(newInput);
if (newInput != input) {
changed = true;
}
}
return changed ? newRel.copy(newRel.getTraitSet(), newInputs) : newRel;
}
```
--
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]
