gemini-code-assist[bot] commented on code in PR #36509:
URL: https://github.com/apache/beam/pull/36509#discussion_r2525195048
##########
sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlCliCatalogTest.java:
##########
@@ -330,4 +332,59 @@ public void
testCreateWriteDropTableWithOtherCatalogScope() {
cli.execute("DROP TABLE catalog_1.db_1.person");
assertNull(metastoreDb1.getTable("person"));
}
+
+ @Test
+ public void testShowCurrentCatalog() {
+ cli.execute("CREATE CATALOG my_catalog TYPE 'local'");
+ cli.execute("CREATE CATALOG my_very_long_catalog_name TYPE 'local'");
+ ByteArrayOutputStream outputStreamCaptor = new ByteArrayOutputStream();
+ System.setOut(new PrintStream(outputStreamCaptor));
+ cli.execute("SHOW CURRENT CATALOG");
+ @SuppressWarnings("DefaultCharset")
+ String printOutput = outputStreamCaptor.toString().trim();
+
+ assertEquals("default (type: local)", printOutput);
+ }
+
+ @Test
+ public void testShowCatalogs() {
+ cli.execute("CREATE CATALOG my_catalog TYPE 'local'");
+ cli.execute("CREATE CATALOG my_very_long_catalog_name TYPE 'local'");
+ ByteArrayOutputStream outputStreamCaptor = new ByteArrayOutputStream();
+ System.setOut(new PrintStream(outputStreamCaptor));
+ cli.execute("SHOW CATALOGS");
+ @SuppressWarnings("DefaultCharset")
+ String printOutput = outputStreamCaptor.toString().trim();
+
+ assertEquals(
+ "+---------------------------------------+\n"
+ + "| Catalog Name | Type |\n"
+ + "+---------------------------------------+\n"
+ + "| default | local |\n"
+ + "| my_catalog | local |\n"
+ + "| my_very_long_catalog_name | local |\n"
+ + "+---------------------------------------+",
+ printOutput);
+ }
+
+ @Test
+ public void testShowCatalogsWithPattern() {
+ cli.execute("CREATE CATALOG my_catalog TYPE 'local'");
+ cli.execute("CREATE CATALOG my_very_long_catalog_name TYPE 'local'");
+ ByteArrayOutputStream outputStreamCaptor = new ByteArrayOutputStream();
+ System.setOut(new PrintStream(outputStreamCaptor));
+ cli.execute("SHOW CATALOGS");
+ @SuppressWarnings("DefaultCharset")
+ String printOutput = outputStreamCaptor.toString().trim();
+
+ assertEquals(
+ "+---------------------------------------+\n"
+ + "| Catalog Name | Type |\n"
+ + "+---------------------------------------+\n"
+ + "| default | local |\n"
+ + "| my_catalog | local |\n"
+ + "| my_very_long_catalog_name | local |\n"
+ + "+---------------------------------------+",
+ printOutput);
+ }
Review Comment:
This test method appears to be a copy of `testShowCatalogs` and does not
test the `LIKE` pattern functionality as its name suggests. The executed
command is `SHOW CATALOGS` without a `LIKE` clause. The test should be updated
to verify the pattern matching functionality.
```java
public void testShowCatalogsWithPattern() {
cli.execute("CREATE CATALOG my_catalog TYPE 'local'");
cli.execute("CREATE CATALOG my_very_long_catalog_name TYPE 'local'");
ByteArrayOutputStream outputStreamCaptor = new ByteArrayOutputStream();
System.setOut(new PrintStream(outputStreamCaptor));
cli.execute("SHOW CATALOGS LIKE '%long%'");
@SuppressWarnings("DefaultCharset")
String printOutput = outputStreamCaptor.toString().trim();
assertEquals(
"+---------------------------------------+\n"
+ "| Catalog Name | Type |\n"
+ "+---------------------------------------+\n"
+ "| my_very_long_catalog_name | local |\n"
+ "+---------------------------------------+",
printOutput);
}
```
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/parser/SqlShowDatabases.java:
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.extensions.sql.impl.parser;
+
+import static
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.util.Static.RESOURCE;
+
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
+import org.apache.beam.sdk.extensions.sql.impl.BeamCalciteSchema;
+import org.apache.beam.sdk.extensions.sql.impl.CatalogManagerSchema;
+import org.apache.beam.sdk.extensions.sql.impl.CatalogSchema;
+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.runtime.SqlFunctions;
+import org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.schema.Schema;
+import org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.sql.SqlCall;
+import
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.sql.SqlIdentifier;
+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.SqlNode;
+import
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.sql.SqlOperator;
+import
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.sql.SqlSpecialOperator;
+import org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.sql.SqlUtil;
+import
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.sql.parser.SqlParserPos;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+public class SqlShowDatabases extends SqlCall implements
BeamSqlParser.ExecutableStatement {
+ private static final SqlOperator OPERATOR =
+ new SqlSpecialOperator("SHOW DATABASES", SqlKind.OTHER_DDL);
+
+ private final boolean showCurrentOnly;
+ private final @Nullable SqlIdentifier catalogName;
+ private final @Nullable SqlNode regex;
+
+ public SqlShowDatabases(
+ SqlParserPos pos,
+ boolean showCurrentOnly,
+ @Nullable SqlIdentifier catalogName,
+ @Nullable SqlNode regex) {
+ super(pos);
+ this.showCurrentOnly = showCurrentOnly;
+ this.catalogName = catalogName;
+ this.regex = regex;
+ }
+
+ @Override
+ public List<SqlNode> getOperandList() {
+ return Collections.emptyList();
+ }
+
+ @Override
+ public SqlOperator getOperator() {
+ return OPERATOR;
+ }
+
+ @Override
+ public void execute(CalcitePrepare.Context context) {
+ Schema schema = SqlDdlNodes.schema(context, true).schema;
+
+ if (!(schema instanceof CatalogManagerSchema)) {
+ throw SqlUtil.newContextException(
+ pos,
+ RESOURCE.internal(
+ "Attempting to execute 'SHOW DATABASES' with unexpected Calcite
Schema of type "
+ + schema.getClass()));
+ }
+
+ CatalogSchema catalogSchema;
+ if (catalogName != null) {
+ String catalog = catalogName.toString();
+ catalogSchema = ((CatalogManagerSchema)
schema).getCatalogSchema(catalog);
+ } else {
+ catalogSchema = ((CatalogManagerSchema)
schema).getCurrentCatalogSchema();
+ }
+
+ if (showCurrentOnly) {
+ @Nullable BeamCalciteSchema currentDatabase =
catalogSchema.getCurrentDatabaseSchema();
+ String output =
+ currentDatabase == null ? "No database is currently set" :
currentDatabase.name();
+ System.out.println(output);
+ return;
+ }
+ Collection<String> databases = catalogSchema.databases();
+ print(databases, catalogSchema.getCatalog().name(),
SqlDdlNodes.getString(regex));
+ }
+
+ private static void print(Collection<String> databases, String path,
@Nullable String pattern) {
+ SqlFunctions.LikeFunction calciteLike = new SqlFunctions.LikeFunction();
+
+ final String headerName = "Databases in " + path;
+ final String separatorChar = "-";
+
+ int nameWidth = headerName.length();
+
+ for (String dbName : databases) {
+ nameWidth = Math.max(nameWidth, dbName.length());
+ }
+
+ nameWidth += 2;
+ String format = "| %-" + nameWidth + "s |%n";
+
+ int separatorWidth = nameWidth + 2;
+ String separator =
+ String.format(
+ "+" + new String(new char[separatorWidth]).replace("\0",
separatorChar) + "+%n");
+
+ System.out.printf(separator);
+ System.out.printf(format, headerName);
+ System.out.printf(separator);
+ if (databases != null) {
+ for (String dbName :
databases.stream().sorted().collect(Collectors.toList())) {
+ if (pattern == null || calciteLike.like(dbName, pattern)) {
+ System.out.printf(format, dbName);
+ }
+ }
+ }
Review Comment:
The `databases()` method on `CatalogSchema` (and its implementations
`InMemoryCatalog` and `IcebergCatalog`) is not expected to return null. It
returns a `Collection<String>`. Therefore, this null check is redundant and can
be removed to simplify the code.
```java
for (String dbName :
databases.stream().sorted().collect(Collectors.toList())) {
if (pattern == null || calciteLike.like(dbName, pattern)) {
System.out.printf(format, dbName);
}
}
```
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/parser/SqlShowCatalogs.java:
##########
@@ -0,0 +1,128 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.extensions.sql.impl.parser;
+
+import static
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.util.Static.RESOURCE;
+
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.List;
+import java.util.stream.Collectors;
+import org.apache.beam.sdk.extensions.sql.impl.CatalogManagerSchema;
+import org.apache.beam.sdk.extensions.sql.meta.catalog.Catalog;
+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.runtime.SqlFunctions;
+import org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.schema.Schema;
+import org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.sql.SqlCall;
+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.SqlNode;
+import
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.sql.SqlOperator;
+import
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.sql.SqlSpecialOperator;
+import org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.sql.SqlUtil;
+import
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.sql.parser.SqlParserPos;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+public class SqlShowCatalogs extends SqlCall implements
BeamSqlParser.ExecutableStatement {
+ private static final SqlOperator OPERATOR =
+ new SqlSpecialOperator("SHOW CATALOGS", SqlKind.OTHER_DDL);
+
+ private final boolean showCurrentOnly;
+ private final @Nullable SqlNode regex;
+
+ public SqlShowCatalogs(SqlParserPos pos, boolean showCurrentOnly, @Nullable
SqlNode regex) {
+ super(pos);
+ this.showCurrentOnly = showCurrentOnly;
+ this.regex = regex;
+ }
+
+ @Override
+ public List<SqlNode> getOperandList() {
+ return Collections.emptyList();
+ }
+
+ @Override
+ public SqlOperator getOperator() {
+ return OPERATOR;
+ }
+
+ @Override
+ public void execute(CalcitePrepare.Context context) {
+ Schema schema = SqlDdlNodes.schema(context, true).schema;
+
+ if (!(schema instanceof CatalogManagerSchema)) {
+ throw SqlUtil.newContextException(
+ pos,
+ RESOURCE.internal(
+ "Attempting execute 'SHOW CATALOGS' with unexpected Calcite
Schema of type "
+ + schema.getClass()));
+ }
+ CatalogManagerSchema managerSchema = ((CatalogManagerSchema) schema);
+ if (showCurrentOnly) {
+ Catalog currentCatalog =
managerSchema.getCurrentCatalogSchema().getCatalog();
+ System.out.printf("%s (type: %s)", currentCatalog.name(),
currentCatalog.type());
+ return;
+ }
+ Collection<Catalog> catalogs = managerSchema.catalogs();
+ print(catalogs, SqlDdlNodes.getString(regex));
+ }
+
+ private static void print(Collection<Catalog> catalogs, @Nullable String
pattern) {
+ SqlFunctions.LikeFunction calciteLike = new SqlFunctions.LikeFunction();
+
+ final String headerName = "Catalog Name";
+ final String headerType = "Type";
+ final String separatorChar = "-";
+
+ int nameWidth = headerName.length();
+ int typeWidth = headerType.length();
+
+ // find the longest string in each column
+ for (Catalog catalog : catalogs) {
+ nameWidth = Math.max(nameWidth, catalog.name().length());
+ typeWidth = Math.max(typeWidth, catalog.type().length());
+ }
+
+ // add a small padding
+ nameWidth += 2;
+ typeWidth += 2;
+
+ // format string with calculated widths for left-justification (%-Ns)
+ String format = "| %-" + nameWidth + "s | %-" + typeWidth + "s |%n";
+
+ // separator width = column widths + padding + separators - corners ('+')
+ int separatorWidth = nameWidth + typeWidth + 5;
+ String separator =
+ String.format(
+ "+" + new String(new char[separatorWidth]).replace("\0",
separatorChar) + "+%n");
Review Comment:
This way of creating a repeated string is a bit clunky. A more modern and
readable approach for Java 8+ would be to use `String.join` with
`Collections.nCopies`. This would also avoid the intermediate `char[]`
allocation and the `replace` call. This comment also applies to
`SqlShowDatabases.java` and `SqlShowTables.java`.
```suggestion
String separator =
String.format(
"+" + String.join("", Collections.nCopies(separatorWidth,
separatorChar)) + "+%n");
```
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/parser/SqlShowTables.java:
##########
@@ -0,0 +1,152 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.extensions.sql.impl.parser;
+
+import static
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.util.Static.RESOURCE;
+
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.List;
+import java.util.stream.Collectors;
+import org.apache.beam.sdk.extensions.sql.impl.BeamCalciteSchema;
+import org.apache.beam.sdk.extensions.sql.impl.CatalogManagerSchema;
+import org.apache.beam.sdk.extensions.sql.impl.CatalogSchema;
+import org.apache.beam.sdk.extensions.sql.impl.TableName;
+import org.apache.beam.sdk.extensions.sql.meta.Table;
+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.runtime.SqlFunctions;
+import org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.schema.Schema;
+import org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.sql.SqlCall;
+import
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.sql.SqlIdentifier;
+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.SqlNode;
+import
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.sql.SqlOperator;
+import
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.sql.SqlSpecialOperator;
+import org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.sql.SqlUtil;
+import
org.apache.beam.vendor.calcite.v1_40_0.org.apache.calcite.sql.parser.SqlParserPos;
+import
org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Splitter;
+import
org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.collect.Lists;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+public class SqlShowTables extends SqlCall implements
BeamSqlParser.ExecutableStatement {
+ private static final SqlOperator OPERATOR =
+ new SqlSpecialOperator("SHOW TABLES", SqlKind.OTHER_DDL);
+ private final @Nullable SqlIdentifier databaseName;
+ private final @Nullable SqlNode regex;
+
+ public SqlShowTables(
+ SqlParserPos pos, @Nullable SqlIdentifier databaseName, @Nullable
SqlNode regex) {
+ super(pos);
+ this.databaseName = databaseName;
+ this.regex = regex;
+ }
+
+ @Override
+ public List<SqlNode> getOperandList() {
+ return Collections.emptyList();
+ }
+
+ @Override
+ public SqlOperator getOperator() {
+ return OPERATOR;
+ }
+
+ @Override
+ public void execute(CalcitePrepare.Context context) {
+ Schema schema = SqlDdlNodes.schema(context, true).schema;
+
+ if (!(schema instanceof CatalogManagerSchema)) {
+ throw SqlUtil.newContextException(
+ pos,
+ RESOURCE.internal(
+ "Attempting to execute 'SHOW TABLES' with unexpected Calcite
Schema of type "
+ + schema.getClass()));
+ }
+
+ CatalogSchema catalogSchema;
+ @Nullable BeamCalciteSchema databaseSchema;
+ if (databaseName != null) {
+ List<String> components =
Lists.newArrayList(Splitter.on(".").split(databaseName.toString()));
+ TableName pathOverride = TableName.create(components, "");
+ catalogSchema =
+ pathOverride.catalog() != null
+ ? ((CatalogManagerSchema) schema).getCatalogSchema(pathOverride)
+ : ((CatalogManagerSchema) schema).getCurrentCatalogSchema();
+
+ databaseSchema = catalogSchema.getDatabaseSchema(pathOverride);
+ } else {
+ catalogSchema = ((CatalogManagerSchema)
schema).getCurrentCatalogSchema();
+ databaseSchema = catalogSchema.getCurrentDatabaseSchema();
+ }
+
+ if (databaseSchema == null) {
+ throw SqlUtil.newContextException(
+ pos,
+ RESOURCE.internal(
+ "Attempting to execute 'SHOW TABLES' with no Database used.
Please set a Database first then re-run."));
+ }
+
+ String path = catalogSchema.getCatalog().name() + "." +
databaseSchema.name();
+ Collection<Table> tables = databaseSchema.getTables();
+ print(tables, path, SqlDdlNodes.getString(regex));
+ }
+
+ private static void print(
+ @Nullable Collection<Table> tables, String path, @Nullable String
pattern) {
+ SqlFunctions.LikeFunction calciteLike = new SqlFunctions.LikeFunction();
+
+ final String headerName = "Tables in " + path;
+ final String headerType = "Type";
+ final String separatorChar = "-";
+
+ int nameWidth = headerName.length();
+ int typeWidth = headerType.length();
+
+ if (tables != null) {
+ for (Table table : tables) {
+ nameWidth = Math.max(nameWidth, table.getName().length());
+ typeWidth = Math.max(typeWidth, table.getType().length());
+ }
+ }
+
+ nameWidth += 2;
+ typeWidth += 2;
+ String format = "| %-" + nameWidth + "s | %-" + typeWidth + "s |%n";
+
+ int separatorWidth = nameWidth + typeWidth + 5;
+ String separator =
+ String.format(
+ "+" + new String(new char[separatorWidth]).replace("\0",
separatorChar) + "+%n");
+
+ System.out.printf(separator);
+ System.out.printf(format, headerName, headerType);
+ System.out.printf(separator);
+ if (tables != null) {
+ for (Table table :
+ tables.stream()
+ .sorted(Comparator.comparing(Table::getName))
+ .collect(Collectors.toList())) {
+ if (pattern == null || calciteLike.like(table.getName(), pattern)) {
+ System.out.printf(format, table.getName(), table.getType());
+ }
+ }
+ System.out.printf(separator);
+ }
Review Comment:
The `getTables()` method on `BeamCalciteSchema` returns
`tableProvider.getTables().values()`, which is not expected to be null. The
`getTables()` method on `TableProvider` returns a `Map`, and `values()` on a
map never returns null. This null check is redundant and can be removed.
```java
for (Table table :
tables.stream()
.sorted(Comparator.comparing(Table::getName))
.collect(Collectors.toList())) {
if (pattern == null || calciteLike.like(table.getName(), pattern)) {
System.out.printf(format, table.getName(), table.getType());
}
}
System.out.printf(separator);
```
--
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]
