yuqi1129 commented on code in PR #14336:
URL: https://github.com/apache/gravitino/pull/14336#discussion_r4409369503
##########
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:
Fixed in 3a119e7560. `TableOperationDispatcher` now rejects a catalog table
with `columns() == null` before it reaches `TableEventDispatcher`: `loadTable`
throws an `IllegalStateException` naming the catalog and the table right after
the catalog call, before importing the table or updating its stored columns. So
`TableInfo` never sees null columns and is left unchanged.
The `toDTO` null guard stays, because `ConnectorObjectSnapshot.detach` calls
`toDTO` inside the catalog call, before the dispatcher check. Without it the
snapshot NPEs first, which is what the new tests hit when I removed it. I added
a comment there.
##########
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:
Agreed, I took the first option: a null column list is rejected at the
dispatcher with a clear error naming the catalog, so `columns: null` never
reaches the wire and the Java and Python clients keep their contract. `toDTO`
keeps its null guard only for the internal connector snapshot (see the reply
above). Also fixed the `TableDTO.Builder.build()` javadoc to list only name and
audit.
##########
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:
Good catch. In 3a119e7560 alter fails too, after the store update: the
catalog has already applied the change, so the store update still runs (e.g. a
table rename is recorded) but keeps the stored columns as they were before the
alter, and then the call throws the same `IllegalStateException`. Failing
before the store update would leave a renamed table out of sync with the store.
So a drop or rename in that alter is not applied to the stored columns, and the
caller gets an error rather than a silent no-op.
`testAlterKeepsStoredColumnsWhenCatalogReportsNoColumns` pins this: the catalog
applied the change, the column ids are unchanged, and the call throws.
##########
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:
Added a comment in 3a119e7560 saying the template delete soft-deletes its
jobs earlier in the same transaction, so a `deleted_at` filter would make this
a no-op.
--
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]
