Copilot commented on code in PR #12010:
URL: https://github.com/apache/gravitino/pull/12010#discussion_r3571357427
##########
core/src/main/java/org/apache/gravitino/catalog/CapabilityHelpers.java:
##########
@@ -507,16 +504,17 @@ private static String applyCapabilitiesOnName(
public static String applyCaseSensitiveOnName(
Capability.Scope scope, String name, Capability capabilities) {
- return capabilities.caseSensitiveOnName(scope).supported() ? name :
name.toLowerCase();
+ return capabilities.normalizeName(scope, name);
Review Comment:
`CapabilityHelpers.applyCaseSensitiveOnName` forwards
`Capability.normalizeName(...)` without validating the result. If a connector
implementation accidentally returns null, this will propagate and later fail
with less actionable NPEs (e.g., in `specificationOnName`). Consider failing
fast with a clear message here.
##########
core/src/main/java/org/apache/gravitino/catalog/CapabilityHelpers.java:
##########
@@ -172,10 +172,7 @@ public static Partition[] applyCaseSensitive(Partition[]
partitions, Capability
}
public static Partition applyCaseSensitive(Partition partition, Capability
capabilities) {
- String newName =
-
capabilities.caseSensitiveOnName(Capability.Scope.PARTITION).supported()
- ? partition.name()
- : partition.name().toLowerCase();
+ String newName = capabilities.normalizeName(Capability.Scope.PARTITION,
partition.name());
if (partition instanceof IdentityPartition) {
Review Comment:
The behavior change (routing partition name normalization through
`Capability.normalizeName`) isn’t covered by a test that proves
`CapabilityHelpers` actually honors an override of `normalizeName` (e.g.,
uppercase fold). Add a unit test using a custom `Capability` that overrides
`normalizeName` and assert `applyCaseSensitive(Partition, ...)` (and ideally
`applyCaseSensitiveOnName`) uses the override.
##########
core/src/main/java/org/apache/gravitino/catalog/CapabilityHelpers.java:
##########
@@ -507,16 +504,17 @@ private static String applyCapabilitiesOnName(
public static String applyCaseSensitiveOnName(
Capability.Scope scope, String name, Capability capabilities) {
- return capabilities.caseSensitiveOnName(scope).supported() ? name :
name.toLowerCase();
+ return capabilities.normalizeName(scope, name);
}
private static String[] applyCaseSensitiveOnColumnName(String[] name,
Capability capabilities) {
- if
(!capabilities.caseSensitiveOnName(Capability.Scope.COLUMN).supported()) {
- String[] standardizeColumnName = Arrays.copyOf(name, name.length);
- standardizeColumnName[0] = name[0].toLowerCase();
- return standardizeColumnName;
+ String normalizedFirstName =
capabilities.normalizeName(Capability.Scope.COLUMN, name[0]);
+ if (normalizedFirstName.equals(name[0])) {
+ return name;
}
Review Comment:
`applyCaseSensitiveOnColumnName` still calls
`capabilities.normalizeName(...)` directly. If `normalizeName` is expected to
never return null (and `applyCaseSensitiveOnName` enforces that), route through
`applyCaseSensitiveOnName` here as well to avoid an immediate NPE in the
`.equals(...)` check and to centralize validation.
##########
core/src/main/java/org/apache/gravitino/catalog/CapabilityHelpers.java:
##########
@@ -172,10 +172,7 @@ public static Partition[] applyCaseSensitive(Partition[]
partitions, Capability
}
public static Partition applyCaseSensitive(Partition partition, Capability
capabilities) {
- String newName =
-
capabilities.caseSensitiveOnName(Capability.Scope.PARTITION).supported()
- ? partition.name()
- : partition.name().toLowerCase();
+ String newName = capabilities.normalizeName(Capability.Scope.PARTITION,
partition.name());
Review Comment:
`applyCaseSensitive(Partition, ...)` calls `capabilities.normalizeName(...)`
directly, bypassing the null-check suggested in `applyCaseSensitiveOnName`.
Using the helper keeps behavior consistent across call sites and provides a
clearer failure mode if a connector returns null.
--
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]
