Copilot commented on code in PR #9938:
URL: https://github.com/apache/gravitino/pull/9938#discussion_r2786271826
##########
catalogs/hive-metastore-common/src/main/java/org/apache/gravitino/hive/converter/HiveDatabaseConverter.java:
##########
@@ -79,17 +78,14 @@ public static Database toHiveDb(HiveSchema hiveSchema) {
Database hiveDb = new Database();
hiveDb.setName(hiveSchema.name());
- Optional.ofNullable(hiveSchema.properties())
- .map(props -> props.get(LOCATION))
- .ifPresent(hiveDb::setLocationUri);
+
Optional.ofNullable(hiveSchema.properties().get(LOCATION)).ifPresent(hiveDb::setLocationUri);
Review Comment:
`hiveSchema.properties()` was previously treated as nullable; the updated
code dereferences it directly and will throw `NullPointerException` when
properties are absent. Restore the prior null-safe handling (wrap
`hiveSchema.properties()` with `Optional.ofNullable(...)` and default to an
empty map when building `parameters`).
##########
api/src/main/java/org/apache/gravitino/rel/indexes/Indexes.java:
##########
@@ -168,7 +165,7 @@ public static class Builder {
* @param indexType The type of the index
* @return The builder for creating a new instance of IndexImpl.
*/
- public Builder withIndexType(IndexType indexType) {
+ public Indexes.IndexImpl.Builder withIndexType(IndexType indexType) {
Review Comment:
These builder methods now use a fully-qualified nested type
(`Indexes.IndexImpl.Builder`) as the return type even though they're already in
the same nested scope. This reduces readability and is inconsistent with
typical builder patterns. Prefer returning `Builder` (or `IndexImpl.Builder`)
here unless there is a concrete name collision requiring qualification.
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/converter/JdbcColumnDefaultValueConverter.java:
##########
@@ -71,14 +61,7 @@ public String fromGravitino(Expression defaultValue) {
if (defaultValue.equals(Literals.NULL)) {
return NULL;
} else if (type instanceof Type.NumericType) {
- String value = literal.value().toString();
- // It seems that literals.value().toString() can be an empty string
for numeric types
- // in some cases like `alter table t modify column `id` int null
default '';`, in such
- // case value is an empty string, we should wrap it with single quotes
to avoid SQL error.
- if (StringUtils.isBlank(value)) {
- value = "'%s'".formatted(value);
- }
- return value;
+ return literal.value().toString();
Review Comment:
The previous handling for numeric literals with blank/empty string values
was removed. If `literal.value().toString()` can be empty (as the removed
comment indicated), SQL generation will emit `DEFAULT ` (invalid SQL). Consider
restoring the blank-value handling (e.g., quoting empty strings or rejecting
invalid numeric default literals) to avoid generating invalid DDL.
```suggestion
Object value = literal.value();
if (value == null) {
throw new IllegalArgumentException("Null is not a valid numeric
default value");
}
String valueStr = value.toString();
if (valueStr.isEmpty()) {
throw new IllegalArgumentException("Empty string is not a valid
numeric default value");
}
return valueStr;
```
##########
clients/client-python/gravitino/exceptions/base.py:
##########
@@ -174,11 +174,7 @@ class NoSuchTagException(NotFoundException):
class TagAlreadyExistsException(AlreadyExistsException):
- """An exception thrown when a tag with specified name already exists."""
-
-
-class TagAlreadyAssociatedException(AlreadyExistsException):
- """Exception thrown when a tag with specified name already associated to a
metadata object."""
+ """An exception thrown when a tag with specified name already associated
to a metadata object."""
Review Comment:
The docstring for `TagAlreadyExistsException` now describes a different
condition (tag already associated to a metadata object). If association is a
distinct error condition, it should remain a separate exception class;
otherwise, update the docstring to match the exception name/meaning so callers
and error handlers aren’t misled.
```suggestion
"""An exception thrown when a tag with specified name already exists."""
```
##########
api/src/main/java/org/apache/gravitino/rel/indexes/Indexes.java:
##########
@@ -190,7 +187,7 @@ public Builder withName(String name) {
* @param fieldNames The field names of the index
* @return The builder for creating a new instance of IndexImpl.
*/
- public Builder withFieldNames(String[][] fieldNames) {
+ public Indexes.IndexImpl.Builder withFieldNames(String[][] fieldNames) {
Review Comment:
These builder methods now use a fully-qualified nested type
(`Indexes.IndexImpl.Builder`) as the return type even though they're already in
the same nested scope. This reduces readability and is inconsistent with
typical builder patterns. Prefer returning `Builder` (or `IndexImpl.Builder`)
here unless there is a concrete name collision requiring qualification.
##########
core/src/main/java/org/apache/gravitino/cache/ReverseIndexCache.java:
##########
@@ -72,7 +72,7 @@ public class ReverseIndexCache {
* or the data will be in the memory forever.
*/
private final Map<EntityCacheKey, List<EntityCacheKey>>
entityToReverseIndexMap =
- Maps.newConcurrentMap();
+ Maps.newHashMap();
Review Comment:
This replaces a concurrent map with a non-thread-safe HashMap for
`entityToReverseIndexMap`. If `ReverseIndexCache` is accessed by multiple
threads (likely for caches), this can cause race conditions and
`ConcurrentModificationException`s. Consider restoring
`Maps.newConcurrentMap()` or guarding all accesses/mutations to this map with
appropriate synchronization.
```suggestion
Maps.newConcurrentMap();
```
##########
catalogs/hive-metastore-common/src/main/java/org/apache/gravitino/hive/converter/HiveDatabaseConverter.java:
##########
@@ -79,17 +78,14 @@ public static Database toHiveDb(HiveSchema hiveSchema) {
Database hiveDb = new Database();
hiveDb.setName(hiveSchema.name());
- Optional.ofNullable(hiveSchema.properties())
- .map(props -> props.get(LOCATION))
- .ifPresent(hiveDb::setLocationUri);
+
Optional.ofNullable(hiveSchema.properties().get(LOCATION)).ifPresent(hiveDb::setLocationUri);
Optional.ofNullable(hiveSchema.comment()).ifPresent(hiveDb::setDescription);
// TODO: Add more privilege info to Hive's Database object after Gravitino
supports privilege.
hiveDb.setOwnerName(hiveSchema.auditInfo().creator());
hiveDb.setOwnerType(PrincipalType.USER);
- Map<String, String> parameters =
-
Optional.ofNullable(hiveSchema.properties()).map(HashMap::new).orElseGet(HashMap::new);
+ Map<String, String> parameters = new HashMap<>(hiveSchema.properties());
Review Comment:
`hiveSchema.properties()` was previously treated as nullable; the updated
code dereferences it directly and will throw `NullPointerException` when
properties are absent. Restore the prior null-safe handling (wrap
`hiveSchema.properties()` with `Optional.ofNullable(...)` and default to an
empty map when building `parameters`).
##########
core/src/main/java/org/apache/gravitino/listener/api/event/ListTopicFailureEvent.java:
##########
@@ -40,15 +39,10 @@ public final class ListTopicFailureEvent extends
TopicFailureEvent {
* @param exception The exception encountered during the attempt to list
topics.
*/
public ListTopicFailureEvent(String user, Namespace namespace, Exception
exception) {
- super(user, toNameIdentifier(namespace), exception);
+ super(user, NameIdentifier.of(namespace.levels()), exception);
Review Comment:
This change removes the prior `Preconditions.checkArgument(namespace !=
null, ...)` guard and will now throw a `NullPointerException` if `namespace` is
ever passed as null. If null is invalid here, reintroduce an explicit argument
check (or keep the helper method) so failures are consistent and easier to
diagnose.
##########
api/src/main/java/org/apache/gravitino/rel/indexes/Indexes.java:
##########
@@ -179,7 +176,7 @@ public Builder withIndexType(IndexType indexType) {
* @param name The name of the index
* @return The builder for creating a new instance of IndexImpl.
*/
- public Builder withName(String name) {
+ public Indexes.IndexImpl.Builder withName(String name) {
Review Comment:
These builder methods now use a fully-qualified nested type
(`Indexes.IndexImpl.Builder`) as the return type even though they're already in
the same nested scope. This reduces readability and is inconsistent with
typical builder patterns. Prefer returning `Builder` (or `IndexImpl.Builder`)
here unless there is a concrete name collision requiring qualification.
##########
catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java:
##########
@@ -291,7 +293,12 @@ private void appendColumnDefinition(JdbcColumn column,
StringBuilder sqlBuilder)
sqlBuilder.append("NOT NULL ");
}
// Add DEFAULT value if specified
- appendDefaultValue(column, sqlBuilder);
+ if (!DEFAULT_VALUE_NOT_SET.equals(column.defaultValue())) {
+ sqlBuilder
+ .append("DEFAULT ")
+
.append(columnDefaultValueConverter.fromGravitino(column.defaultValue()))
+ .append(SPACE);
+ }
Review Comment:
The DEFAULT-value rendering behavior in SQL generation is modified here, but
the previously present SQL-generation unit test file for PostgreSQL was removed
in this revert. To comply with the project’s testing requirement and prevent
regressions (e.g., empty-string defaults and non-set defaults), add/restore
focused unit tests covering this code path.
--
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]
