Copilot commented on code in PR #4815:
URL: https://github.com/apache/polaris/pull/4815#discussion_r3430449817
##########
runtime/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
@@ -91,9 +91,20 @@ static class SchemaInputOptions {
@CommandLine.Option(
names = {"-v", "--schema-version"},
paramLabel = "<schema version>",
+ converter = SchemaVersionConverter.class,
description = "The version of the schema to load in [1, 2, 3,
LATEST].")
Integer schemaVersion;
}
+
+ static class SchemaVersionConverter implements
CommandLine.ITypeConverter<Integer> {
+ @Override
+ public Integer convert(String value) {
+ if ("LATEST".equalsIgnoreCase(value)) {
+ return null;
+ }
+ return Integer.valueOf(value);
+ }
+ }
Review Comment:
Returning `null` from a Picocli `ITypeConverter` makes the option appear to
be provided but with a null value, which can be fragile if any downstream code
treats `schemaVersion` as non-null when `-v/--schema-version` is explicitly
specified. Prefer converting `LATEST` to an explicit sentinel value (e.g.,
`-1`) or switch the option type to something that can represent `LATEST`
explicitly (e.g., an enum or `String` parsed/validated later) so the runtime
behavior is unambiguous.
##########
persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/JdbcBootstrapUtilsTest.java:
##########
@@ -171,5 +171,20 @@ void whenSchemaOptionsIsNull_shouldReturnDefault() {
int result =
JdbcBootstrapUtils.getRequestedSchemaVersion(mockBootstrapOptions);
assertEquals(-1, result);
}
+
+ @Test
+ void latestResolvesToLatestSchemaVersion() {
+ // Simulates --schema-version LATEST: converter returns null →
Optional.ofNullable(null)
+
when(mockSchemaOptions.schemaVersion()).thenReturn(Optional.ofNullable(null));
+ int requested =
JdbcBootstrapUtils.getRequestedSchemaVersion(mockBootstrapOptions);
Review Comment:
`Optional.ofNullable(null)` is always `Optional.empty()`, so this stub is
equivalent to `thenReturn(Optional.empty())` and reads oddly/misleadingly.
Consider stubbing `Optional.empty()` directly and adjusting the comment to
describe the intended state (schema version not present after conversion)
rather than the implementation detail.
--
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]
