jerryshao commented on code in PR #13484:
URL: https://github.com/apache/gravitino/pull/13484#discussion_r4130360029
##########
core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java:
##########
@@ -741,24 +754,84 @@ public static FilesetPO updateFilesetPOWithVersion(
.withDeletedAt(DEFAULT_DELETED_AT)
.build())
.collect(Collectors.toList());
- return FilesetPO.builder()
- .withFilesetId(newFileset.id())
- .withFilesetName(newFileset.name())
- .withMetalakeId(oldFilesetPO.getMetalakeId())
- .withCatalogId(oldFilesetPO.getCatalogId())
- .withSchemaId(oldFilesetPO.getSchemaId())
- .withType(newFileset.filesetType().name())
-
.withAuditInfo(JsonUtils.anyFieldMapper().writeValueAsString(newFileset.auditInfo()))
+ return newFilesetPOBuilder(oldFilesetPO, newFileset)
.withCurrentVersion(currentVersion)
.withLastVersion(currentVersion)
- .withDeletedAt(DEFAULT_DELETED_AT)
+ .withOccVersion(occVersion)
.withFilesetVersionPOs(newFilesetVersionPOs)
.build();
} catch (JsonProcessingException e) {
throw new RuntimeException("Failed to serialize json object:", e);
}
}
+ private static FilesetPO.Builder newFilesetPOBuilder(
+ FilesetPO oldFilesetPO, FilesetEntity newFileset) throws
JsonProcessingException {
+ return FilesetPO.builder()
+ .withFilesetId(newFileset.id())
+ .withFilesetName(newFileset.name())
+ .withMetalakeId(oldFilesetPO.getMetalakeId())
+ .withCatalogId(oldFilesetPO.getCatalogId())
+ .withSchemaId(oldFilesetPO.getSchemaId())
+ .withType(newFileset.filesetType().name())
+
.withAuditInfo(JsonUtils.anyFieldMapper().writeValueAsString(newFileset.auditInfo()))
+ .withDeletedAt(DEFAULT_DELETED_AT);
+ }
+
+ /**
+ * Tells whether an alter leaves every field {@code fileset_version_info}
stores untouched.
+ *
+ * <p>Compares exactly the persisted columns: comment, properties, and the
storage locations.
+ * Properties are compared by value, because the same map can serialize in a
different key order
+ * after a read/write round trip. This decides only whether to write a
snapshot, never whether a
+ * concurrent write happened, which is what the OCC version is for; a wrong
{@code false} costs
+ * one redundant snapshot, the behaviour every alter used to have.
+ *
+ * @param oldFilesetPO the row being replaced, carrying the snapshot its
current version points at
+ * @param newFileset the updated fileset
+ * @param newProperties the updated properties, already serialized
+ * @return true when no stored field changed and no new snapshot is needed
+ */
+ private static boolean filesetSnapshotUnchanged(
+ FilesetPO oldFilesetPO, FilesetEntity newFileset, String newProperties) {
+ List<FilesetVersionPO> storedVersions =
oldFilesetPO.getFilesetVersionPOs();
+ if (storedVersions == null || storedVersions.isEmpty()) {
+ // Nothing to point at, so the alter has to write a snapshot whatever it
changed.
+ return false;
+ }
+ Map<String, String> storedLocations =
+ storedVersions.stream()
+ .collect(
+ Collectors.toMap(
+ FilesetVersionPO::getLocationName,
FilesetVersionPO::getStorageLocation));
+ if (!storedLocations.equals(newFileset.storageLocations())) {
+ return false;
+ }
+ return storedVersions.stream()
+ .allMatch(
+ version ->
+ Objects.equals(version.getFilesetComment(),
newFileset.comment())
+ && filesetPropertiesUnchanged(
+ version.getProperties(), newProperties,
newFileset.properties()));
Review Comment:
[Nit] The comment and properties checks run once per storage location, but
every row in `storedVersions` belongs to the same version, so they are N
identical comparisons — and on the fallback path, N identical JSON parses.
Every read that populates `getFilesetVersionPOs()` joins `ON fm.fileset_id =
vi.fileset_id AND fm.current_version = vi.version`
(`FilesetMetaBaseSQLProvider.java:130`, `:153`, `:229`), so all rows in the
list share one `version` and therefore one `fileset_comment` and one
`properties` string. `storageLocations` is the only per-row field, and it is
already compared as a whole map above. So when the JSON strings differ — which
is the case the new fallback exists for, i.e. every rename of a fileset whose
properties went through a `HashMap` —
`JsonUtils.anyFieldMapper().readValue(...)` at `:829` parses the same string
once per location.
Suggestion: compare comment and properties once against
`storedVersions.get(0)` and drop the `allMatch`, keeping the `storedLocations`
map comparison as the per-row check. Same answer, one parse. Not a correctness
issue, and a fileset has few locations, so this is purely about saying once
what is true once.
Verified by: reading `filesetSnapshotUnchanged` and
`filesetPropertiesUnchanged` in full (`POConverters.java:796-833`) and the
three join conditions that build the list, in this run.
--
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]
