yuqi1129 commented on code in PR #14336:
URL: https://github.com/apache/gravitino/pull/14336#discussion_r4410399599
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -404,8 +412,10 @@ public Table alterTable(NameIdentifier ident,
TableChange... changes)
ident, newIdent);
}
- return EntityCombinedTable.of(alteredTable, updatedTableEntity)
- .withHiddenProperties(catalogResult.hiddenProperties);
+ return checkColumnsReported(
+ ident,
+ EntityCombinedTable.of(alteredTable, updatedTableEntity)
+ .withHiddenProperties(catalogResult.hiddenProperties));
Review Comment:
Took the second option in 2807621631, without touching `TableInfo`: on a
stored table the alter now completes and reports the stored columns (the ones
`updateColumnsIfNecessary` kept), with a warning. So the hook rename and the
success event run, and no null list reaches `TableInfo` or the wire. A column
drop or rename in that alter is still not reflected in the stored columns.
Unstored and managed tables have no stored columns to report, so they still
fail as before; an unstored table has no Gravitino privileges for the hook to
rename. `testRenameCompletesWhenCatalogReportsNoColumns` covers the rename, and
the PR description is updated.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/JobMetaService.java:
##########
@@ -256,7 +258,13 @@ void deleteJobWithVersion(NameIdentifier ident, JobPO
observed) {
mapper ->
mapper.softDeleteJobByRunIdWithVersion(
observed.jobRunId(),
observed.currentVersion())),
- () -> writeFailure(ident, observed)));
+ () -> writeFailure(ident, observed)),
+ () ->
+ SessionUtils.doWithoutCommit(
+ OwnerMetaMapper.class,
+ mapper ->
+ mapper.softDeleteOwnerRelByMetadataObjectIdAndType(
+ observed.jobRunId(), MetadataObject.Type.JOB.name())));
Review Comment:
Deliberate. The retention path removes jobs in bulk by timeline and then
hard-deletes them, and the orphan relation GC is built for that case: it maps
`JOB` to `job_run_meta.job_run_id` and also collects rows after the hard
delete. Cleaning there would need a timeline-scoped owner statement per
backend, and it is a different path from the explicit deletes this PR fixes. A
job owner on an aged-out job is also not visible to users. So I left it to the
GC.
##########
common/src/main/java/org/apache/gravitino/dto/rel/TableDTO.java:
##########
@@ -309,7 +309,7 @@ public S withIndex(IndexDTO[] indexes) {
* Builds a Table DTO based on the provided builder parameters.
*
* @return A new TableDTO instance.
- * @throws IllegalArgumentException If required fields name, columns and
audit are not set.
+ * @throws IllegalArgumentException If required fields name and audit are
not set.
Review Comment:
Done in 2807621631: `@Nullable` on the field, the builder field, `columns()`
and `withColumns`, and the javadoc says null is only for the internal connector
snapshot and a DTO sent to clients always has columns.
--
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]
