Copilot commented on code in PR #10090:
URL: https://github.com/apache/gravitino/pull/10090#discussion_r2867155744
##########
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:
This change can throw a NullPointerException if `hiveSchema.properties()` is
null. Previously, the code handled null safely via
`Optional.ofNullable(hiveSchema.properties())`. Restore the null-safe handling
(e.g., guard `properties()` before calling `.get(...)` and constructing the
`HashMap`).
##########
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:
This change can throw a NullPointerException if `hiveSchema.properties()` is
null. Previously, the code handled null safely via
`Optional.ofNullable(hiveSchema.properties())`. Restore the null-safe handling
(e.g., guard `properties()` before calling `.get(...)` and constructing the
`HashMap`).
##########
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:
Replacing the concurrent map with a plain HashMap makes
`entityToReverseIndexMap` non-thread-safe. Since reverse-index caches are
typically accessed/updated concurrently (and the class already uses
`ConcurrentRadixTree`), this can introduce data races, lost updates, and
`ConcurrentModificationException`. Use a concurrent map implementation (e.g.,
`Maps.newConcurrentMap()` or `new ConcurrentHashMap<>()`) to preserve thread
safety.
```suggestion
Maps.newConcurrentMap();
```
##########
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 no longer matches the exception name/semantics:
`TagAlreadyExistsException` implies name collision, but the text describes an
association conflict (previously handled by `TagAlreadyAssociatedException`).
Update the docstring (or reintroduce a dedicated exception) so error reporting
remains accurate.
```suggestion
"""An exception thrown when a tag with specified name already exists."""
```
##########
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);
this.namespace = namespace;
}
Review Comment:
This change removes the explicit argument validation and changes the failure
mode from a clear `IllegalArgumentException(\"namespace must not be null\")` to
a likely `NullPointerException` if `namespace` is null. Keeping the explicit
precondition check here would preserve a more actionable error message and
stable behavior.
##########
catalogs/hive-metastore-common/src/main/java/org/apache/gravitino/hive/converter/HiveDatabaseConverter.java:
##########
@@ -58,7 +58,6 @@ public static HiveSchema fromHiveDB(Database db) {
.build();
return hiveSchema;
}
-
/**
* Add a comment on lines L57 to L65Add diff commentMarkdown input: edit mode
* selected.WritePreviewHeadingBoldItalicQuoteCodeLinkUnordered listNumbered
listTask
Review Comment:
These lines look like an accidental paste of GitHub UI text into JavaDoc and
should be removed or replaced with meaningful documentation.
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/authentication/kerberos/KerberosClient.java:
##########
@@ -55,7 +54,7 @@ public KerberosClient(
this.refreshCredentials = refreshCredentials;
}
- public UserGroupInformation login(String keytabFilePath) throws IOException {
+ public String login(String keytabFilePath) throws IOException {
Review Comment:
The `login` method now returns a realm string (principal component) rather
than a `UserGroupInformation`, but the name/signature still reads like an
authentication call returning a logged-in identity/context. This is a confusing
contract change for callers. Consider either (a) returning the
`UserGroupInformation` again, and exposing realm via a separate accessor, or
(b) renaming the method to reflect the returned value (e.g.,
`loginAndGetRealm`) and adding/adjusting JavaDoc accordingly.
##########
clients/client-python/gravitino/api/tag/tag.py:
##########
@@ -69,7 +69,7 @@ def name(self) -> str:
Returns:
str: The name of the tag.
"""
- raise NotImplementedError()
+ pass
Review Comment:
These methods are marked with `@abstractmethod`, but using `pass` makes the
default implementation silently return `None` if `super()` is accidentally
invoked from a subclass. Prefer `raise NotImplementedError()` in abstract
method bodies to fail fast and produce clearer errors.
##########
core/src/main/java/org/apache/gravitino/auxiliary/GravitinoAuxiliaryService.java:
##########
@@ -38,10 +38,8 @@ public interface GravitinoAuxiliaryService {
/**
* @param config GravitinoServer will pass the config with prefix
* `gravitino.auxService.{shortName}.` to aux server
- * @param auxMode whether the service is running as an auxiliary service
(true) or standalone
- * (false)
*/
- void serviceInit(Map<String, String> config, boolean auxMode);
+ void serviceInit(Map<String, String> config);
Review Comment:
Changing the public interface method signature from `serviceInit(Map<String,
String>, boolean)` to `serviceInit(Map<String, String>)` is a breaking change
for all existing auxiliary service implementations. If backward compatibility
is required, consider providing a default method overload (or deprecating the
old signature first) and keeping the boolean parameter until
callers/implementations migrate.
```suggestion
/**
* Initializes the auxiliary service with the given configuration and an
additional boolean flag.
*
* <p>This method is kept for backward compatibility with existing
auxiliary service
* implementations that use the older two-argument signature. New
implementations should prefer
* {@link #serviceInit(Map)}.
*
* @param config GravitinoServer will pass the config with prefix
* {@code gravitino.auxService.{shortName}.} to aux server
* @param enabled an additional boolean flag preserved for backward
compatibility; its value is
* ignored by the default implementation
*/
@Deprecated
default void serviceInit(Map<String, String> config, boolean enabled) {
serviceInit(config);
}
```
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/authentication/kerberos/KerberosClient.java:
##########
@@ -95,7 +93,7 @@ public UserGroupInformation login(String keytabFilePath)
throws IOException {
TimeUnit.SECONDS);
}
- return kerberosLoginUgi;
+ return principalComponents.get(1);
Review Comment:
The `login` method now returns a realm string (principal component) rather
than a `UserGroupInformation`, but the name/signature still reads like an
authentication call returning a logged-in identity/context. This is a confusing
contract change for callers. Consider either (a) returning the
`UserGroupInformation` again, and exposing realm via a separate accessor, or
(b) renaming the method to reflect the returned value (e.g.,
`loginAndGetRealm`) and adding/adjusting JavaDoc accordingly.
##########
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:
Previously, blank numeric defaults were wrapped in quotes to avoid
generating invalid SQL (e.g., `DEFAULT `). With the current change, an
empty-string numeric literal will emit an empty token and likely produce
invalid SQL. Consider restoring the blank/empty handling for numeric literals
(at minimum, guard against empty values and serialize them safely).
```suggestion
String numericValue = literal.value().toString();
if (numericValue.isEmpty()) {
// Guard against empty numeric defaults to avoid generating
invalid SQL like "DEFAULT ".
// Historically, blank numeric defaults were wrapped in quotes.
return "''";
}
return numericValue;
```
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/converter/JdbcColumnDefaultValueConverter.java:
##########
@@ -37,18 +34,11 @@ public class JdbcColumnDefaultValueConverter {
protected static final String CURRENT_TIMESTAMP = "CURRENT_TIMESTAMP";
protected static final String NULL = "NULL";
protected static final DateTimeFormatter DATE_TIME_FORMATTER =
- new DateTimeFormatterBuilder()
- .appendPattern("yyyy-MM-dd HH:mm:ss")
- .appendFraction(ChronoField.NANO_OF_SECOND, 0, 6, true)
- .toFormatter();
+ DateTimeFormatter.ofPattern("yyyy-MM-dd
HH:mm:ss[.SSSSSS][.SSSSS][.SSSS][.SSS][.SS][.S]");
protected static final DateTimeFormatter DATE_FORMATTER =
DateTimeFormatter.ofPattern("yyyy-MM-dd");
-
protected static final DateTimeFormatter TIME_FORMATTER =
- new DateTimeFormatterBuilder()
- .appendPattern("HH:mm:ss")
- .appendFraction(ChronoField.NANO_OF_SECOND, 0, 6, true)
- .toFormatter();
+
DateTimeFormatter.ofPattern("HH:mm:ss[.SSSSSS][.SSSSS][.SSSS][.SSS][.SS][.S]");
public String fromGravitino(Expression defaultValue) {
if (DEFAULT_VALUE_NOT_SET.equals(defaultValue)) {
Review Comment:
Test coverage regressed around default-value SQL generation: multiple SQL
generation tests were removed (e.g., MySQL/PostgreSQL default-value tests)
while default-value serialization logic changed here. Add/restore unit tests
that cover (1) empty string defaults, (2) non-empty string defaults, and (3)
edge cases for numeric literals to ensure generated SQL remains valid across
dialects.
##########
catalogs/catalog-jdbc-oceanbase/src/test/java/org/apache/gravitino/catalog/oceanbase/operation/TestOceanBaseTableOperations.java:
##########
@@ -43,10 +43,12 @@
import org.apache.gravitino.utils.RandomNameUtils;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Tag;
import org.junit.jupiter.api.Test;
@Tag("gravitino-docker-test")
+@Disabled
public class TestOceanBaseTableOperations extends TestOceanBase {
Review Comment:
Marking the entire docker test class `@Disabled` effectively removes
validation for OceanBase behavior in CI. If these tests are flaky/unsupported
in some environments, prefer conditional execution (e.g.,
`Assumptions.assumeTrue(...)` with a clear env flag) or keep them enabled under
the existing `gravitino-docker-test` tag so they can still run in appropriate
pipelines.
##########
common/src/main/java/org/apache/gravitino/config/ConfigConstants.java:
##########
@@ -83,12 +83,9 @@ private ConfigConstants() {}
/** The version number for the 1.1.0 release. */
public static final String VERSION_1_1_0 = "1.1.0";
- /** The version number for the 1.1.1 release. */
- public static final String VERSION_1_1_1 = "1.1.1";
-
/** The version number for the 1.2.0 release. */
public static final String VERSION_1_2_0 = "1.2.0";
/** The current version of backend storage initialization script. */
- public static final String CURRENT_SCRIPT_VERSION = VERSION_1_2_0;
+ public static final String CURRENT_SCRIPT_VERSION = VERSION_1_1_0;
Review Comment:
Downgrading `CURRENT_SCRIPT_VERSION` from a newer value to `VERSION_1_1_0`
can cause schema initialization/migration logic to select the wrong script
baseline. If the latest scripts are actually 1.2.0+, this will break upgrades
or leave the backend under-migrated. Confirm the intended current script
version and keep this aligned with the newest shipped initialization scripts.
```suggestion
public static final String CURRENT_SCRIPT_VERSION = VERSION_1_2_0;
```
--
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]
