gemini-code-assist[bot] commented on code in PR #38951:
URL: https://github.com/apache/beam/pull/38951#discussion_r3431283665
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/provider/text/TextTableProvider.java:
##########
@@ -238,7 +238,7 @@ public PCollection<String> expand(PCollection<Row> input) {
/** Write-side converter for {@link TextTable} with format {@code 'csv'}. */
@VisibleForTesting
- static class RowToCsv extends PTransform<PCollection<Row>,
PCollection<String>>
+ public static class RowToCsv extends PTransform<PCollection<Row>,
PCollection<String>>
Review Comment:
Since `RowToCsv` is now being made `public` specifically to allow external
integration with text table serialization, keeping the `@VisibleForTesting`
annotation is contradictory and misleading. It should be removed so that users
and static analysis tools do not treat it as an internal-only API.
```java
public static class RowToCsv extends PTransform<PCollection<Row>,
PCollection<String>>
```
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/CalciteQueryPlanner.java:
##########
@@ -203,13 +268,81 @@ 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.");
+ boolean success = false;
+ 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);
+ success = true;
+ 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);
+ } finally {
+ if (!success) {
+ planner.close();
+ }
+ }
+ }
+
+ @Override
+ public BeamRelNode convertToBeamRel(RelNode relNode, QueryParameters
queryParameters)
+ throws SqlConversionException {
+ boolean success = false;
Review Comment:
To enforce defensive programming and prevent unexpected
`NullPointerException`s, we should add explicit null checks for `relNode` and
`queryParameters` at the entry point of this public API.
```java
@Override
public BeamRelNode convertToBeamRel(RelNode relNode, QueryParameters
queryParameters)
throws SqlConversionException {
Preconditions.checkNotNull(relNode, "relNode cannot be null");
Preconditions.checkNotNull(queryParameters, "queryParameters cannot be
null");
boolean success = false;
```
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/CalciteQueryPlanner.java:
##########
@@ -203,13 +268,81 @@ 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.");
+ boolean success = false;
Review Comment:
To enforce defensive programming, we should add an explicit null check for
`sqlStatement`. Additionally, the error message for query parameters is
slightly misleading because the Calcite dialect *does* support query
parameters, but they must be supplied during physical planning
(`convertToBeamRel`) rather than logical planning (`parseToRel`). Let's update
the message to be more precise and helpful to users of this new API.
```java
@Override
public RelNode parseToRel(String sqlStatement, QueryParameters
queryParameters)
throws ParseException, SqlConversionException {
Preconditions.checkNotNull(sqlStatement, "sqlStatement cannot be null");
Preconditions.checkArgument(
queryParameters.getKind() == Kind.NONE,
"Query parameters are not supported during logical plan parsing;
please provide them when converting to a Beam physical plan instead.");
boolean success = false;
```
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/BeamSqlEnv.java:
##########
@@ -116,6 +119,41 @@ 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) throws
SqlConversionException {
+ return convertToBeamRel(relNode, QueryParameters.ofNone());
+ }
+
+ public BeamRelNode convertToBeamRel(RelNode relNode, QueryParameters
queryParameters)
+ throws SqlConversionException {
+ return planner.convertToBeamRel(relNode, queryParameters);
+ }
+
+ public RelNode parseLogicalPlan(String query) throws ParseException,
SqlConversionException {
+ return parseLogicalPlan(query, QueryParameters.ofNone());
+ }
+
+ public RelNode parseLogicalPlan(String query, QueryParameters
queryParameters)
+ throws ParseException, SqlConversionException {
+ return planner.parseToRel(query, queryParameters);
+ }
+
+ public void registerSchemaFunction(String name, Function function) {
+ connection.getCurrentSchemaPlus().add(name, function);
+ }
Review Comment:
To prevent potential `NullPointerException`s when registering schema
functions, we should add defensive null checks for `name` and `function`.
```suggestion
public void registerSchemaFunction(String name, Function function) {
java.util.Objects.requireNonNull(name, "name cannot be null");
java.util.Objects.requireNonNull(function, "function cannot be null");
connection.getCurrentSchemaPlus().add(name, function);
}
```
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/BeamSqlEnv.java:
##########
@@ -116,6 +119,41 @@ 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) throws
SqlConversionException {
+ return convertToBeamRel(relNode, QueryParameters.ofNone());
+ }
+
+ public BeamRelNode convertToBeamRel(RelNode relNode, QueryParameters
queryParameters)
+ throws SqlConversionException {
+ return planner.convertToBeamRel(relNode, queryParameters);
+ }
+
+ public RelNode parseLogicalPlan(String query) throws ParseException,
SqlConversionException {
+ return parseLogicalPlan(query, QueryParameters.ofNone());
+ }
+
+ public RelNode parseLogicalPlan(String query, QueryParameters
queryParameters)
+ throws ParseException, SqlConversionException {
+ return planner.parseToRel(query, queryParameters);
+ }
+
+ public void registerSchemaFunction(String name, Function function) {
+ connection.getCurrentSchemaPlus().add(name, function);
+ }
+
+ public
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.sql.SqlOperatorTable
+ getOperatorTable() {
+ return planner.getOperatorTable();
+ }
Review Comment:
Using the fully qualified name
`org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.sql.SqlOperatorTable`
in the method signature reduces readability. Since other Calcite classes are
imported, we should use the simple name `SqlOperatorTable` instead.
```java
public SqlOperatorTable getOperatorTable() {
return planner.getOperatorTable();
}
```
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/BeamSqlEnv.java:
##########
@@ -47,8 +47,10 @@
import org.apache.beam.sdk.transforms.SerializableFunction;
import
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.jdbc.CalcitePrepare;
import
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.plan.RelOptUtil;
+import org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.rel.RelNode;
import
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.schema.Function;
import org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.sql.SqlKind;
+import
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.tools.RelBuilder;
Review Comment:
Let's import `SqlOperatorTable` here so we can avoid using its fully
qualified name in the method signature of `getOperatorTable()`, which improves
code readability.
```suggestion
import org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.sql.SqlKind;
import
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.sql.SqlOperatorTable;
import
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.tools.RelBuilder;
```
--
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]
