gemini-code-assist[bot] commented on code in PR #38951:
URL: https://github.com/apache/beam/pull/38951#discussion_r3406032220
##########
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:
The `planner.convertToBeamRel` method throws `SqlConversionException`, which
is a checked exception. Since this exception is not caught here,
`convertToBeamRel` must declare `throws SqlConversionException` in its
signature to avoid a compilation error.
```suggestion
public BeamRelNode convertToBeamRel(RelNode relNode) throws
SqlConversionException {
return planner.convertToBeamRel(relNode, 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 overridden `convertToBeamRel` method must declare `throws
SqlConversionException` to match the `QueryPlanner` interface definition and to
propagate the checked exception thrown by the helper method.
```java
@Override
public BeamRelNode convertToBeamRel(RelNode relNode, QueryParameters
queryParameters)
throws SqlConversionException {
return convertToBeamRel(relNode, (RelCollation) null);
}
```
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/CalciteQueryPlanner.java:
##########
@@ -224,26 +339,23 @@ public BeamRelNode convertToBeamRel(String sqlStatement,
QueryParameters queryPa
RelMetadataQuery.THREAD_PROVIDERS.set(
JaninoRelMetadataProvider.of(relNode.getCluster().getMetadataProvider()));
relNode.getCluster().invalidateMetadataQuery();
- beamRelNode = (BeamRelNode) planner.transform(0, desiredTraits, relNode);
+ Program program = config.getPrograms().get(0);
+ LOG.info("Desired traits: {}", desiredTraits);
+ beamRelNode =
+ program.run(
+ relNode.getCluster().getPlanner(),
+ relNode,
+ desiredTraits,
+ ImmutableList.of(),
+ ImmutableList.of());
LOG.info("BEAMPlan>\n{}", BeamSqlRelUtils.explainLazily(beamRelNode));
- } catch (RelConversionException | CannotPlanException e) {
+ } catch (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);
+ String.format("Unable to convert relNode to Beam: %s", relNode), e);
} finally {
planner.close();
}
- 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);
+ return (BeamRelNode) beamRelNode;
Review Comment:
To prevent potential `IndexOutOfBoundsException` and `ClassCastException`,
we should defensively check if the programs list is empty and verify that the
optimized node is indeed an instance of `BeamRelNode` before casting.
```java
if (config.getPrograms().isEmpty()) {
throw new SqlConversionException("No planning programs configured in
FrameworkConfig.");
}
Program program = config.getPrograms().get(0);
LOG.info("Desired traits: {}", desiredTraits);
RelNode optimizedNode =
program.run(
relNode.getCluster().getPlanner(),
relNode,
desiredTraits,
ImmutableList.of(),
ImmutableList.of());
LOG.info("BEAMPlan>\n{}",
BeamSqlRelUtils.explainLazily(optimizedNode));
if (!(optimizedNode instanceof BeamRelNode)) {
throw new SqlConversionException(
String.format(
"The optimizer was unable to produce a Beam physical plan. "
+ "Expected BeamRelNode, but got: %s",
optimizedNode.getClass().getName()));
}
beamRelNode = (BeamRelNode) optimizedNode;
} catch (CannotPlanException e) {
throw new SqlConversionException(
String.format("Unable to convert relNode to Beam: %s", relNode),
e);
} finally {
planner.close();
}
return beamRelNode;
```
##########
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` (a checked
exception) in addition to `ParseException`. This method must declare `throws
ParseException, SqlConversionException` to compile successfully.
```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);
+ }
+
+ private BeamRelNode convertToBeamRel(RelNode relNode, @Nullable RelCollation
collation) {
Review Comment:
This helper method throws `SqlConversionException` (a checked exception) and
must declare it in its `throws` clause.
```suggestion
private BeamRelNode convertToBeamRel(RelNode relNode, @Nullable
RelCollation collation)
throws SqlConversionException {
```
##########
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);
+ }
+
+ private BeamRelNode convertToBeamRel(RelNode relNode, @Nullable RelCollation
collation) {
+ RelNode beamRelNode;
Review Comment:
Declaring `beamRelNode` as `BeamRelNode` directly avoids the need for
casting at the end of the method.
```suggestion
BeamRelNode beamRelNode;
```
--
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]
