gemini-code-assist[bot] commented on code in PR #38951:
URL: https://github.com/apache/beam/pull/38951#discussion_r3406102831
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/BeamSqlEnv.java:
##########
@@ -116,6 +122,31 @@ public BeamRelNode parseQuery(String query,
QueryParameters queryParameters)
return planner.convertToBeamRel(query, queryParameters);
}
+ public QueryPlanner getPlanner() {
+ return planner;
+ }
+
+ public RelBuilder getRelBuilder() {
+ return planner.getRelBuilder();
+ }
+
+ public BeamRelNode convertToBeamRel(RelNode relNode) {
+ return planner.convertToBeamRel(relNode, QueryParameters.ofNone());
+ }
Review Comment:
To allow external callers to pass query parameters when converting an
externally constructed or parsed `RelNode` to a `BeamRelNode`, please expose an
overloaded `convertToBeamRel` method that accepts `QueryParameters`.
```suggestion
public BeamRelNode convertToBeamRel(RelNode relNode) {
return convertToBeamRel(relNode, QueryParameters.ofNone());
}
public BeamRelNode convertToBeamRel(RelNode relNode, QueryParameters
queryParameters) {
return planner.convertToBeamRel(relNode, queryParameters);
}
```
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/BeamSqlEnv.java:
##########
@@ -116,6 +122,31 @@ public BeamRelNode parseQuery(String query,
QueryParameters queryParameters)
return planner.convertToBeamRel(query, queryParameters);
}
+ public QueryPlanner getPlanner() {
+ return planner;
+ }
+
+ public RelBuilder getRelBuilder() {
+ return planner.getRelBuilder();
+ }
+
+ public BeamRelNode convertToBeamRel(RelNode relNode) {
+ return planner.convertToBeamRel(relNode, QueryParameters.ofNone());
+ }
+
+ public RelNode parseLogicalPlan(String query) throws ParseException {
+ return planner.parseToRel(query, QueryParameters.ofNone());
+ }
Review Comment:
The `planner.parseToRel` method throws `SqlConversionException`, which is a
checked exception. Since `parseLogicalPlan` does not catch or declare
`SqlConversionException` in its `throws` clause, this will cause a compilation
error.
Please update the method signature to declare `throws
SqlConversionException`.
```suggestion
public RelNode parseLogicalPlan(String query) throws ParseException,
SqlConversionException {
return planner.parseToRel(query, QueryParameters.ofNone());
}
```
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/CalciteQueryPlanner.java:
##########
@@ -203,13 +272,59 @@ 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);
+ }
+
+ @Override
+ public RelNode parseToRel(String sqlStatement, QueryParameters
queryParameters)
+ throws ParseException, SqlConversionException {
+ Preconditions.checkArgument(
+ queryParameters.getKind() == Kind.NONE,
+ "Beam SQL Calcite dialect does not yet support query parameters.");
+ try {
+ SqlNode parsed = planner.parse(sqlStatement);
+ TableResolutionUtils.setupCustomTableResolution(connection, parsed);
+ SqlNode validated = planner.validate(parsed);
+ // root of original logical plan
+ RelRoot root = planner.rel(validated);
+ return root.rel;
+ } catch (RelConversionException 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);
+ }
+ }
+
+ @Override
+ public BeamRelNode convertToBeamRel(RelNode relNode, QueryParameters
queryParameters) {
+ return convertToBeamRel(relNode, (RelCollation) null);
+ }
Review Comment:
The `convertToBeamRel(RelNode, QueryParameters)` implementation currently
ignores the `queryParameters` argument. If the logical plan contains
dynamic/positional parameters, they will not be bound, leading to runtime
failures or incorrect physical plans.
Please update the implementation to bind positional parameters using
`ParameterBinder` if they are provided.
```java
@Override
public BeamRelNode convertToBeamRel(RelNode relNode, QueryParameters
queryParameters) {
if (queryParameters.getKind() == Kind.POSITIONAL) {
relNode =
bindParameters(
relNode,
new ParameterBinder(relNode.getCluster().getRexBuilder(),
queryParameters));
}
return convertToBeamRel(relNode, (RelCollation) null);
}
```
--
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]
