Copilot commented on code in PR #7307:
URL: https://github.com/apache/kyuubi/pull/7307#discussion_r2693418151
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/dialect/StarRocksDialect.scala:
##########
@@ -26,4 +26,8 @@ class StarRocksDialect extends MySQLDialect {
override def getSchemaHelper(): SchemaHelper = new StarRocksSchemaHelper
+
+ override def getCatalogsOperation(): String = {
+ return "show catalogs"
+ }
Review Comment:
Explicit return statements are not idiomatic in Scala. Remove the 'return'
keyword and let the expression be the implicit return value.
```suggestion
override def getCatalogsOperation(): String = "show catalogs"
```
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/dialect/MySQLDialect.scala:
##########
@@ -36,6 +36,10 @@ class MySQLDialect extends JdbcDialect {
statement
}
+ override def getSchemasOperation(catalog: String, schema: String): String = {
+ return "select database()"
Review Comment:
The method accepts catalog and schema parameters but doesn't use them. This
implementation ignores the parameters entirely, which may not be the expected
behavior for all use cases. Consider documenting this behavior or validating if
the parameters should influence the query.
```suggestion
// MySQLDialect currently returns only the current database as the
schema.
// The catalog and schema parameters are accepted for interface
compatibility
// but do not influence the generated query.
if (StringUtils.isNotBlank(catalog) || StringUtils.isNotBlank(schema)) {
// Parameters are intentionally ignored; behavior is to always return
the current database.
}
"select database()"
```
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/dialect/MySQLDialect.scala:
##########
@@ -36,6 +36,10 @@ class MySQLDialect extends JdbcDialect {
statement
}
+ override def getSchemasOperation(catalog: String, schema: String): String = {
+ return "select database()"
Review Comment:
Explicit return statements are not idiomatic in Scala. Remove the 'return'
keyword and let the expression be the implicit return value.
```suggestion
"select database()"
```
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/dialect/MySQLDialect.scala:
##########
@@ -60,8 +64,11 @@ class MySQLDialect extends JdbcDialect {
filters += s"$TABLE_CATALOG = '$catalog'"
}
- if (StringUtils.isNotBlank(schema)) {
+ // when DBeaver connect use %, kyuubi to return all tables from all other
databases.
+ if (StringUtils.isNotBlank(schema) && !"%".equals(schema)) {
filters += s"$TABLE_SCHEMA LIKE '$schema'"
+ } else {
+ filters += s"$TABLE_SCHEMA = database()"
Review Comment:
The condition combines two separate cases: when schema is blank and when
schema is '%'. This logic would be clearer if split into separate conditions to
distinguish between 'schema is blank' and 'schema is %' cases. Consider
restructuring as: if blank -> database(), else if '%' -> database(), else ->
LIKE schema.
```suggestion
if (StringUtils.isBlank(schema)) {
// No schema provided: default to current database
filters += s"$TABLE_SCHEMA = database()"
} else if ("%".equals(schema)) {
// '%' means all schemas: do not add TABLE_SCHEMA filter
} else {
filters += s"$TABLE_SCHEMA LIKE '$schema'"
```
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/dialect/MySQLDialect.scala:
##########
@@ -60,8 +64,11 @@ class MySQLDialect extends JdbcDialect {
filters += s"$TABLE_CATALOG = '$catalog'"
}
- if (StringUtils.isNotBlank(schema)) {
+ // when DBeaver connect use %, kyuubi to return all tables from all other
databases.
Review Comment:
Corrected grammar in comment: 'use' should be 'uses', and the sentence
structure should be improved for clarity. Suggested: 'when DBeaver connects
using %, Kyuubi returns all tables from all databases.'
```suggestion
// when DBeaver connects using %, Kyuubi returns all tables from all
databases.
```
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/dialect/StarRocksDialect.scala:
##########
@@ -26,4 +26,8 @@ class StarRocksDialect extends MySQLDialect {
override def getSchemaHelper(): SchemaHelper = new StarRocksSchemaHelper
+
Review Comment:
There is an unnecessary blank line before this method. Remove the extra
blank line to maintain consistent spacing with other methods in the class.
```suggestion
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
