Copilot commented on code in PR #9793:
URL: https://github.com/apache/gravitino/pull/9793#discussion_r2720123722
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceRESTServiceIT.java:
##########
@@ -500,12 +500,14 @@ void testCreateTable() throws IOException, ApiException {
Assertions.assertEquals("v1", response.getProperties().get("key1"));
Assertions.assertEquals("value_a", response.getStorageOptions().get("a"));
Assertions.assertEquals("value_b", response.getStorageOptions().get("b"));
+ Assertions.assertNotNull(response.getVersion());
DescribeTableRequest describeTableRequest = new DescribeTableRequest();
describeTableRequest.setId(ids);
DescribeTableResponse loadTable = ns.describeTable(describeTableRequest);
Assertions.assertNotNull(loadTable);
Assertions.assertEquals(location, loadTable.getLocation());
+ Assertions.assertNotNull(loadTable.getVersion());
Review Comment:
These assertions only check `version` is non-null, but don’t verify it’s
propagated correctly or remains consistent across APIs (e.g., `createTable`
version matches `describeTable` version). Strengthen the test by asserting
equality between the two versions (and optionally that it’s >= 0) so
regressions in version handling are caught.
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java:
##########
@@ -92,7 +93,10 @@ public DescribeTableResponse describeTable(
response.setProperties(table.properties());
response.setLocation(table.properties().get(LANCE_LOCATION));
response.setSchema(toJsonArrowSchema(table.columns()));
- response.setVersion(null);
+ response.setVersion(
+ Optional.ofNullable(table.properties().get(LANCE_TABLE_VERSION))
+ .map(Long::valueOf)
+ .orElse(null));
Review Comment:
`Long::valueOf` will throw `NumberFormatException` if the stored
`lance.version` property is non-numeric (e.g., legacy tables, manual edits, or
corrupted metadata), which would turn a describe request into a 500. Consider
parsing defensively (catch `NumberFormatException`) and either returning `null`
or mapping to a user-facing error with context.
```suggestion
Long versionValue = null;
String versionStr = table.properties().get(LANCE_TABLE_VERSION);
if (versionStr != null) {
try {
versionValue = Long.valueOf(versionStr);
} catch (NumberFormatException e) {
// Ignore invalid numeric value and leave version as null.
}
}
response.setVersion(versionValue);
```
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java:
##########
@@ -147,7 +151,10 @@ public CreateTableResponse createTable(
// Extract storage options from table properties. All storage options
stores in table
// properties.
response.setStorageOptions(LancePropertiesUtils.getLanceStorageOptions(t.properties()));
- response.setVersion(null);
+ response.setVersion(
+ Optional.ofNullable(t.properties().get(LANCE_TABLE_VERSION))
+ .map(Long::valueOf)
Review Comment:
Same parsing concern here: `Long::valueOf` can throw if `lance.version` is
not a valid long, causing create responses to fail unexpectedly. Prefer
defensive parsing (catch `NumberFormatException`) so table creation doesn’t
regress on older/invalid metadata.
```suggestion
.map(
v -> {
try {
return Long.valueOf(v);
} catch (NumberFormatException e) {
return null;
}
})
```
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java:
##########
@@ -285,8 +288,21 @@ Table createTableInternal(
new
WriteParams.Builder().withStorageOptions(storageProps).build())) {
// Only create the table metadata in Gravitino after the Lance dataset
is successfully
// created.
+ long datasetVersion = ignored.version();
+ Map<String, String> updatedProperties =
+ ImmutableMap.<String, String>builder()
+ .putAll(properties)
+ .put(LanceConstants.LANCE_TABLE_VERSION,
String.valueOf(datasetVersion))
+ .build();
Review Comment:
Persisting the dataset version only at create-time can become incorrect
after later dataset mutations (e.g., `alterTable` adds indexes by opening the
dataset and calling `dataset.createIndex(...)`, which may advance the dataset
version). To keep `version` accurate per the REST contract (“current/max
version”), consider reading the current version from the underlying dataset
when serving describe/load responses, or updating `lance.version` in metadata
whenever the dataset is modified.
--
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]
