Copilot commented on code in PR #9909:
URL: https://github.com/apache/gravitino/pull/9909#discussion_r2773223971
##########
core/src/main/java/org/apache/gravitino/meta/FunctionEntity.java:
##########
@@ -163,18 +139,13 @@ public boolean equals(Object o) {
&& Objects.equals(comment, that.comment)
&& functionType == that.functionType
&& deterministic == that.deterministic
- && Objects.equals(returnType, that.returnType)
- && Arrays.equals(returnColumns, that.returnColumns)
&& Arrays.equals(definitions, that.definitions)
&& Objects.equals(auditInfo, that.auditInfo);
}
@Override
public int hashCode() {
- int result =
- Objects.hash(
- id, name, namespace, comment, functionType, deterministic,
returnType, auditInfo);
- result = 31 * result + Arrays.hashCode(returnColumns);
+ int result = Objects.hash(id, name, namespace, comment, functionType,
deterministic, auditInfo);
Review Comment:
The hashCode implementation should maintain consistency with equals. Since
the equals method compares definitions using Arrays.equals, the hashCode should
include Arrays.hashCode(definitions) to ensure equal objects have equal hash
codes.
##########
core/src/main/java/org/apache/gravitino/authorization/RoleManager.java:
##########
@@ -66,7 +64,6 @@ RoleEntity createRole(
Map<String, String> properties,
List<SecurableObject> securableObjects)
throws RoleAlreadyExistsException {
Review Comment:
The removal of metalake validation checks (checkMetalake) from role
management methods should be covered by tests to ensure the new behavior is
correct and doesn't introduce authorization bypass issues.
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/converter/JdbcColumnDefaultValueConverter.java:
##########
@@ -61,7 +71,14 @@ public String fromGravitino(Expression defaultValue) {
if (defaultValue.equals(Literals.NULL)) {
return NULL;
} else if (type instanceof Type.NumericType) {
- return literal.value().toString();
+ 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);
+ }
Review Comment:
The comment mentions a specific case with numeric types having empty string
defaults, but wrapping empty strings with single quotes for numeric columns
would result in invalid SQL. This logic should check the column type and only
apply string quoting for string/character types, not numeric types.
--
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]
