Skip to content

Commit 473ef06

Browse files
committed
[Storage] Address review feedback for UC token-based getCommits
- Compare table UUIDs via UUID equality instead of case-sensitive string compare, matching the existing UUID.fromString pattern in commit(). - Extract fromDeltaCommit helper to mirror UCTokenBasedRestClient's fromDeltaCommitInfo and remove the inline null-check storm inside the Commit/FileStatus constructor. - Document why version filtering is client-side (loadTable does not expose server-side filters). - Drop the dead requireNonNull(response, ...) after a successful loadTable; the SDK never returns null on 2xx.
1 parent 153b88a commit 473ef06

1 file changed

Lines changed: 29 additions & 24 deletions

File tree

storage/src/main/java/io/delta/storage/commit/uccommitcoordinator/UCDeltaTokenBasedRestClient.java

Lines changed: 29 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -294,6 +294,7 @@ public GetCommitsResponse getCommits(
294294
Objects.requireNonNull(startVersion, "startVersion must not be null");
295295
Objects.requireNonNull(endVersion, "endVersion must not be null");
296296

297+
UUID expectedTableUuid = UUID.fromString(tableId);
297298
String[] namespace = Objects.requireNonNull(
298299
tableIdentifier.getNamespace(), "tableIdentifier namespace must not be null");
299300
if (namespace.length != 2) {
@@ -305,6 +306,9 @@ public GetCommitsResponse getCommits(
305306
String table = Objects.requireNonNull(tableIdentifier.getName(), "table name must not be null");
306307
String fullName = catalog + "." + schema + "." + table;
307308

309+
// The UC loadTable endpoint does not support server-side filtering by version range, so
310+
// we fetch the full unbackfilled commit window and filter client-side below. The server
311+
// bounds the window size, so this list is not unbounded in practice.
308312
LoadTableResponse response;
309313
try {
310314
response = deltaTablesApi.loadTable(catalog, schema, table);
@@ -319,18 +323,15 @@ public GetCommitsResponse getCommits(
319323
e);
320324
}
321325

322-
Objects.requireNonNull(response, "loadTable response must not be null");
323-
String actualTableId = response.getMetadata() != null
324-
&& response.getMetadata().getTableUuid() != null
325-
? response.getMetadata().getTableUuid().toString()
326-
: null;
327-
if (!Objects.equals(tableId, actualTableId)) {
326+
TableMetadata metadata = response.getMetadata();
327+
UUID actualTableUuid = metadata != null ? metadata.getTableUuid() : null;
328+
if (!expectedTableUuid.equals(actualTableUuid)) {
328329
throw new InvalidTargetTableException(
329330
String.format(
330331
"Table UUID mismatch for %s: expected %s but got %s",
331332
fullName,
332-
tableId,
333-
actualTableId));
333+
expectedTableUuid,
334+
actualTableUuid));
334335
}
335336

336337
Path basePath = CoordinatedCommitsUtils.commitDirPath(
@@ -346,22 +347,7 @@ public GetCommitsResponse getCommits(
346347
if (endVersion.isPresent() && version > endVersion.get()) {
347348
continue;
348349
}
349-
350-
commits.add(new Commit(
351-
version,
352-
new FileStatus(
353-
Objects.requireNonNull(
354-
deltaCommit.getFileSize(), "commit fileSize must not be null"),
355-
false /* isdir */,
356-
0 /* block_replication */,
357-
0 /* blocksize */,
358-
Objects.requireNonNull(
359-
deltaCommit.getFileModificationTimestamp(),
360-
"commit fileModificationTimestamp must not be null"),
361-
new Path(basePath, Objects.requireNonNull(
362-
deltaCommit.getFileName(), "commit fileName must not be null"))),
363-
Objects.requireNonNull(
364-
deltaCommit.getTimestamp(), "commit timestamp must not be null")));
350+
commits.add(fromDeltaCommit(deltaCommit, basePath));
365351
}
366352
}
367353

@@ -370,6 +356,25 @@ public GetCommitsResponse getCommits(
370356
return new GetCommitsResponse(commits, latestTableVersion);
371357
}
372358

359+
/** Converts a UC SDK {@link DeltaCommit} to a Delta {@link Commit}. */
360+
private Commit fromDeltaCommit(DeltaCommit deltaCommit, Path basePath) {
361+
FileStatus fileStatus = new FileStatus(
362+
Objects.requireNonNull(
363+
deltaCommit.getFileSize(), "commit fileSize must not be null"),
364+
false /* isdir */,
365+
0 /* block_replication */,
366+
0 /* blocksize */,
367+
Objects.requireNonNull(
368+
deltaCommit.getFileModificationTimestamp(),
369+
"commit fileModificationTimestamp must not be null"),
370+
new Path(basePath, Objects.requireNonNull(
371+
deltaCommit.getFileName(), "commit fileName must not be null")));
372+
return new Commit(
373+
Objects.requireNonNull(deltaCommit.getVersion(), "commit version must not be null"),
374+
fileStatus,
375+
Objects.requireNonNull(deltaCommit.getTimestamp(), "commit timestamp must not be null"));
376+
}
377+
373378
@Override
374379
public void finalizeCreate(
375380
String tableName,

0 commit comments

Comments
 (0)