FANNG1 commented on PR #11060:
URL: https://github.com/apache/gravitino/pull/11060#issuecomment-4504103482
## Review: handling of new lance-namespace fields
I reviewed how this PR handles the fields introduced between lance-namespace
`0.4.5` and `0.7.5`. Most of the new fields (`CreateTable` query params,
`DescribeTableResponse.properties/managed_versioning/is_only_declared`,
`DescribeTableRequest.check_declared`,
`DeclareTableRequest/Response.properties`, `AlterColumnsEntry.rename` ->
`JsonNullable<String>`) are handled correctly. A few concerns below.
### Main issue: `is_only_declared` semantics do not match the Lance spec
`GravitinoLanceTableOperations.describeTable()` fills `is_only_declared` by
reading the `lance.declared` property:
```java
if (checkDeclared) {
response.setIsOnlyDeclared(
Boolean.parseBoolean(table.properties().getOrDefault(LANCE_TABLE_DECLARED,
"false")));
}
```
But `lance.declared`:
- is written exactly once, inside `declareTable()`;
- is registered as **immutable** in `LanceTableDelegator`, so it can never
change afterwards.
The Lance spec (v0.7.0) defines `is_only_declared` as a **current state** —
"the table is only declared in the namespace and has no underlying storage
data/metadata files yet" — and expects it to flip to `false` once the table is
materialized by the first data write.
Since the lance-rest-server does not implement `InsertIntoTable`,
materialization happens client-side (e.g. lance-ray writes directly to storage)
and the namespace never learns about it, so `lance.declared` stays `true`
forever. **As a result, a table that was declared and then populated with real
data will still report `is_only_declared=true` from
`describeTable(check_declared=true)`, which is incorrect.**
Suggestion: when `check_declared=true`, probe the table location for an
actual Lance manifest instead of reading the static property — or at minimum
document this limited semantics for Gravitino.
### `ListTables` / `ListAllTables` ignore `include_declared` (v0.7.1)
`ListTablesRequest` has an `includeDeclared` field in 0.7.5, but
`LanceNamespaceOperations.listTables` only accepts `delimiter / page_token /
limit`. A client that explicitly sends `include_declared=false` is silently
ignored and still receives declared-only tables.
### `lance.create-empty` -> `lance.declared` is a silent breaking change
The property key is renamed, but this key is persisted in Gravitino's
metadata store. After an upgrade, existing tables carrying
`lance.create-empty=true` will no longer be recognized by the `declaredOnly`
check. The PR description states "No user-facing change", which is not accurate
for existing deployments. Suggest a compatibility fallback:
```java
boolean declaredOnly = Boolean.parseBoolean(
properties.getOrDefault(LANCE_TABLE_DECLARED,
properties.getOrDefault(LANCE_TABLE_CREATE_EMPTY, "false")));
```
### Minor: `DescribeTableResponse.properties` exposes internal properties
`describeTable` assigns `table.properties()` to both `metadata` and
`properties`. Per the v0.5.0 semantics, `properties` is meant to be
namespace-managed business-logic properties, whereas `table.properties()`
contains internal implementation details (`lance.creation-mode`,
`lance.declared`, ...). Consider filtering before returning.
### Minor: `createTable` query-parameter precedence is undocumented
`props` (headers) -> `queryProperties` -> `queryStorageOptions` are applied
via successive `putAll`, so query parameters silently override header values.
Worth noting the precedence in the Javadoc.
--
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]
