gemini-code-assist[bot] commented on code in PR #38880:
URL: https://github.com/apache/beam/pull/38880#discussion_r3383790429
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/CalciteQueryPlanner.java:
##########
@@ -225,6 +237,15 @@ public BeamRelNode convertToBeamRel(String sqlStatement,
QueryParameters queryPa
return beamRelNode;
}
+ 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 `bindParameters` method attempts to call `rel.accept(binder)` where
`binder` is a `RexShuttle`. However, in Apache Calcite, `RelNode` only has an
`accept(RelShuttle)` method, and `RexShuttle` does not implement `RelShuttle`.
This will cause a compilation error.
To apply a `RexShuttle` to a `RelNode`'s expressions, you should use a
`RelShuttle` that visits the `RelNode` and applies the `RexShuttle` to its
expressions (for example, by overriding `visit` methods or using
`rel.accept(new RelShuttleImpl() { ... })`). Since doing this manually for all
relational operators is verbose, you can use `rel.accept(new RelShuttleImpl() {
... })` and override the `visit` methods, or use a custom `RelShuttle`
implementation that handles expression transformation.
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/CalciteQueryPlanner.java:
##########
@@ -191,28 +196,35 @@ public BeamRelNode convertToBeamRel(String sqlStatement,
QueryParameters queryPa
// root of original logical plan
RelRoot root = planner.rel(validated);
+ RelNode relNode = root.rel;
+ if (queryParameters.getKind() == Kind.POSITIONAL) {
+ relNode =
+ bindParameters(
+ relNode,
+ new ParameterBinder(root.rel.getCluster().getRexBuilder(),
queryParameters));
+ }
LOG.info("SQLPlan>\n{}", BeamSqlRelUtils.explainLazily(root.rel));
Review Comment:
The logging statement still prints the original plan `root.rel` which
contains unbound dynamic parameters (`?`). It would be more helpful to log the
plan after parameter binding (`relNode`) so that the fully resolved query plan
is visible in the logs.
```suggestion
LOG.info("SQLPlan>\n{}", BeamSqlRelUtils.explainLazily(relNode));
```
--
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]
