jerryshao commented on code in PR #14336:
URL: https://github.com/apache/gravitino/pull/14336#discussion_r4408486081
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -977,12 +977,23 @@ private Pair<Boolean, List<ColumnEntity>>
updateColumnsIfNecessary(
return Pair.of(false, Collections.emptyList());
}
+ // A catalog that reports no column list says nothing about the columns,
so keep the stored
+ // ones. Treating it as "all columns dropped" would delete their tags,
policies and owners.
+ if (tableFromCatalog.columns() == null) {
Review Comment:
[Question] This guard also changes `alterTable`, which the description and
the new test do not cover.
At line 372 `updateColumnsIfNecessary` is called with the altered table and
its `getRight()` is fed straight into `withColumns(...)` (line 382) while the
boolean is ignored. So when a catalog returns no column list from `alterTable`,
the stored columns are now kept exactly as they were, including columns the
alter just dropped or renamed, where previously they were cleared. The
`nameChanges` argument is also dropped on this path, since the early return
never looks at it.
Keeping them beats deleting every column's tags and policies, but it means
an explicitly requested drop or rename is silently not applied to the store. Is
that the intended behaviour for alter, or should alter treat a missing column
list from the catalog as an error rather than a no-op? Either answer is
defensible; a test through `alterTable` would pin whichever you pick.
Verified by: reading the alter path at
`TableOperationDispatcher.java:360-392` (it uses `getRight()` and never
consults `getLeft()`) and the load path at `:1097-1110` (it returns early when
`getLeft()` is false) on this branch.
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/OwnerMetaBaseSQLProvider.java:
##########
@@ -284,6 +285,20 @@ public String
softDeleteOwnerRelByCatalogId(@Param("catalogId") Long catalogId)
+ ")";
}
+ public String softDeleteOwnerRelOfJobsByTemplateId(@Param("jobTemplateId")
Long jobTemplateId) {
+ String now = currentTimestampMillisExpression();
+ return "UPDATE "
+ + OWNER_TABLE_NAME
+ + " SET deleted_at = "
+ + now
+ + ", updated_at = "
+ + now
+ + " WHERE deleted_at = 0 AND metadata_object_type = 'JOB' AND
metadata_object_id IN ("
+ + " SELECT job_run_id FROM "
+ + JobMetaMapper.TABLE_NAME
+ + " WHERE job_template_id = #{jobTemplateId})";
Review Comment:
[Nit] The subquery correctly has no `deleted_at = 0` filter on
`job_run_meta`, because `softDeleteJobsByTemplateId` has already soft-deleted
those rows earlier in the same transaction (`JobTemplateMetaService.java:264`,
one lambda before `deleteJobTemplateRelations` on line 265). Adding the filter
that nearly every other statement in this file carries would turn the cleanup
into a silent no-op.
That is an easy thing for someone to "fix" later for consistency, and no
test would be obviously about it. One line of comment here saying the job rows
are expected to be already soft-deleted would protect it.
Verified by: reading the operation order in `deleteJobTemplateWithVersion`
(`JobTemplateMetaService.java:239-269`) on this branch.
##########
common/src/main/java/org/apache/gravitino/dto/util/DTOConverters.java:
##########
@@ -299,7 +299,11 @@ public static TableDTO toDTO(Table table) {
.withName(table.name())
.withComment(table.comment())
.withColumns(
-
Arrays.stream(table.columns()).map(DTOConverters::toDTO).toArray(ColumnDTO[]::new))
+ table.columns() == null
+ ? null
Review Comment:
[Important] Even past the NPE above, `columns: null` on the wire is not
consumable by either shipped client, so this trades a server-side failure for a
client-side one rather than a successful load.
- Java: `TableResponse.validate()` checks only name, audit and partitioning
(`common/src/main/java/org/apache/gravitino/dto/responses/TableResponse.java:60-69`),
so a null-columns payload passes validation; `RelationalTable.columns()` then
does `Arrays.stream(table.columns())` with no null check
(`clients/client-java/src/main/java/org/apache/gravitino/client/RelationalTable.java:156`),
giving an NPE in the caller's process.
- Python: `TableDTO.__post_init__` asserts `columns is not None and
len(columns) > 0` with "columns cannot be null or empty"
(`clients/client-python/gravitino/dto/rel/table_dto.py:105-113`), so
deserializing the response raises before the user ever sees the table.
Either keep rejecting a null column list at the boundary and fail with a
clear message naming the catalog, or make null an explicit part of the contract
and update both clients plus `TableResponse.validate()` here. Doing half of it
leaves the one state that cannot work.
Verified by: reading both client sources and `TableResponse.java` on this
branch; `TableDTO.Builder.build()` confirms nothing server-side rejects null
columns any more (it checks only name and audit).
##########
common/src/main/java/org/apache/gravitino/dto/util/DTOConverters.java:
##########
@@ -299,7 +299,11 @@ public static TableDTO toDTO(Table table) {
.withName(table.name())
.withComment(table.comment())
.withColumns(
-
Arrays.stream(table.columns()).map(DTOConverters::toDTO).toArray(ColumnDTO[]::new))
+ table.columns() == null
Review Comment:
[Important] This guard does not make the load succeed; it moves the NPE one
layer out, so the stated user-facing fix does not happen.
`GravitinoEnv.java:1114` wraps the table dispatcher in
`TableEventDispatcher`, and `TableEventDispatcher.loadTable` builds `new
TableInfo(table)` for every successful load (`TableEventDispatcher.java:112`).
`TableInfo`'s constructor does `this.columns = columns.clone()`
(`TableInfo.java:94`) with no null check, while `partitions`, `sortOrders` and
`indexes` immediately below it (lines 97-100) are all null-guarded. On the
server, loading a table whose catalog reports `columns() == null` therefore
still fails with an NPE in the listener layer, before `toDTO` is ever reached.
Suggest null-guarding `TableInfo`'s columns the way the sibling arrays
already are (and marking the field `@Nullable`) as part of this PR, since the
stated fix depends on that layer.
Verified by: reading `TableInfo.java:93-100`,
`TableEventDispatcher.java:105-124` and `GravitinoEnv.java:1114` on this
branch. The new test calls `tableOperationDispatcher.loadTable` directly
(`TestTableOperationDispatcher.java:1527`), which is why it passes without
crossing the event dispatcher.
--
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]
