gemini-code-assist[bot] commented on code in PR #35718:
URL: https://github.com/apache/beam/pull/35718#discussion_r2237775325
##########
buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy:
##########
@@ -1832,21 +1832,20 @@ class BeamModulePlugin implements Plugin<Project> {
project.ext.includeInJavaBom = configuration.publish
project.ext.exportJavadoc = configuration.exportJavadoc
- boolean publishEnabledByCommand = isRelease(project) ||
project.hasProperty('publishing')
if (forkJavaVersion == '') {
// project needs newer version and not served.
// If not publishing ,disable the project. Otherwise, fail the build
def msg = "project ${project.name} needs newer Java version to
compile. Consider set -Pjava${project.javaVersion}Home"
- if (publishEnabledByCommand) {
- throw new GradleException("Publish enabled but " + msg + ".")
+ if (isRelease(project)) {
+ throw new GradleException("Release enabled but " + msg + ".")
} else {
Review Comment:
This hunk introduces a logic change that seems unrelated to the Calcite
version upgrade. The condition for throwing a `GradleException` when a required
Java version is missing has been changed. While this change might be desirable,
including it in a large dependency upgrade PR makes it harder to review and
understand its impact. It would be better to extract this logic change into a
separate, focused pull request. This will improve clarity and make the history
easier to follow.
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/CalciteQueryPlanner.java:
##########
@@ -141,6 +142,9 @@
final SqlOperatorTable opTab0 =
connection.config().fun(SqlOperatorTable.class,
SqlStdOperatorTable.instance());
+ // Revert the flag flip of CALCITE-3870 which led to missing rules
+ SqlToRelConverter.Config sqlToRelConfig =
SqlToRelConverter.config().withExpand(true);
Review Comment:
This change reverts a flag flip in CALCITE-3870. It would be helpful to add
a comment explaining why this reversion is necessary for Beam SQL.
##########
sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlDslSqlStdOperatorsTest.java:
##########
@@ -708,8 +709,8 @@
@Test
@SqlOperatorTests({
- @SqlOperatorTest(name = "CHARACTER_LENGTH", kind = "OTHER_FUNCTION"),
- @SqlOperatorTest(name = "CHAR_LENGTH", kind = "OTHER_FUNCTION"),
+ @SqlOperatorTest(name = "CHARACTER_LENGTH", kind = "CHAR_LENGTH"),
Review Comment:
The `kind` for `CHARACTER_LENGTH` and `CHAR_LENGTH` should be `CHAR_LENGTH`
instead of `OTHER_FUNCTION`.
```suggestion
@SqlOperatorTest(name = "CHARACTER_LENGTH", kind = "CHAR_LENGTH"),
```
##########
sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlDslSqlStdOperatorsTest.java:
##########
@@ -311,7 +312,7 @@ public void testArithmeticOperator() {
.addExpr("c_double + c_bigint", 2.0)
.addExpr("1 - 1", 0)
.addExpr("1.0 - 1", ZERO_1)
Review Comment:
The result of `1 - 0.0` should be `0.1` instead of `1.0`.
```suggestion
.addExpr("1 - 0.0", ONE_1)
```
##########
sdks/java/extensions/sql/build.gradle:
##########
@@ -164,11 +164,15 @@ task copyFmppTemplatesFromCalciteCore(type: Copy) {
into "${project.buildDir}/templates-fmpp"
filter{
line ->
- line.replace('import org.apache.calcite.', 'import
org.apache.beam.vendor.calcite.v1_28_0.org.apache.calcite.')
+ line.replace('import org.apache.calcite.', 'import
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.')
}
filter{
line ->
- line.replace('import static org.apache.calcite.', 'import static
org.apache.beam.vendor.calcite.v1_28_0.org.apache.calcite.')
+ line.replace('import static org.apache.calcite.', 'import static
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.')
+ }
+ filter{
+ line ->
+ line.replace('import com.google.common.', 'import
org.apache.beam.vendor.calcite.v1_40_0.com.google.common.')
Review Comment:
For better readability and a minor performance improvement, these three
separate `filter` closures can be combined into a single one.
```
filter { line ->
line.replace('import org.apache.calcite.', 'import
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.')
.replace('import static org.apache.calcite.', 'import static
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.')
.replace('import com.google.common.', 'import
org.apache.beam.vendor.calcite.v1_40_0.com.google.common.')
}
```
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/CalciteQueryPlanner.java:
##########
@@ -249,7 +254,7 @@ public RelOptCost getNonCumulativeCost(RelNode rel,
RelMetadataQuery mq) {
// here and based on the design we also need to remove the cached values
// We need to first remove the cached values.
- List<Table.Cell<RelNode, List, Object>> costKeys =
+ List<Table.Cell<RelNode, ?, Object>> costKeys =
bmq.map.cellSet().stream()
.filter(entry -> entry.getValue() instanceof BeamCostModel)
.filter(entry -> ((BeamCostModel) entry.getValue()).isInfinite())
Review Comment:
The type parameter for `costKeys` is too restrictive. It should be `?`
instead of `List`.
```suggestion
List<Table.Cell<RelNode, ?, Object>> costKeys =
```
##########
sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlDslSqlStdOperatorsTest.java:
##########
@@ -1129,8 +1130,7 @@
}
@Test
- // https://github.com/apache/beam/issues/19001
- // @SqlOperatorTest(name = "FLOOR", kind = "FLOOR")
+ @SqlOperatorTest(name = "FLOOR", kind = "FLOOR")
Review Comment:
This test was commented out. It should be uncommented.
```suggestion
@SqlOperatorTest(name = "FLOOR", kind = "FLOOR")
```
##########
sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlDslSqlStdOperatorsTest.java:
##########
@@ -1140,14 +1140,14 @@
.addExpr("FLOOR(ts TO DAY)",
parseTimestampWithUTCTimeZone("1986-02-15 00:00:00"))
.addExpr("FLOOR(ts TO MONTH)",
parseTimestampWithUTCTimeZone("1986-02-01 00:00:00"))
.addExpr("FLOOR(ts TO YEAR)",
parseTimestampWithUTCTimeZone("1986-01-01 00:00:00"))
- .addExpr("FLOOR(c_double)", 1.0);
+ .addExpr("FLOOR(c_double)", 1.0)
+ .addExpr("FLOOR(-c_double)", -2.0);
Review Comment:
Add a test case for `FLOOR(-c_double)`.
```suggestion
.addExpr("FLOOR(c_double)", 1.0)
.addExpr("FLOOR(-c_double)", -2.0);
```
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/CalciteQueryPlanner.java:
##########
@@ -141,6 +142,9 @@
final SqlOperatorTable opTab0 =
connection.config().fun(SqlOperatorTable.class,
SqlStdOperatorTable.instance());
+ // Revert the flag flip of CALCITE-3870 which led to missing rules
+ SqlToRelConverter.Config sqlToRelConfig =
SqlToRelConverter.config().withExpand(true);
Review Comment:
Consider adding a comment to explain why `withExpand(true)` is needed here.
```suggestion
// Revert the flag flip of CALCITE-3870 which led to missing rules
SqlToRelConverter.Config sqlToRelConfig =
SqlToRelConverter.config().withExpand(true); // Needed for...
```
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/CalciteConnectionWrapper.java:
##########
@@ -333,8 +334,10 @@ public void setSchema(String schema) throws SQLException {
connection.setSchema(schema);
}
+ // CalciteConnection.getSchema() marked nullable but Connection.getSchema()
does not
+ @SuppressWarnings("override.return")
@Override
- public String getSchema() throws SQLException {
+ public @Nullable String getSchema() throws SQLException {
Review Comment:
Adding `@Nullable` annotation to this method is a good defensive coding
practice, as it explicitly documents that this method can return null. However,
it is important to ensure that all call sites are prepared to handle a null
return value.
--
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]
