Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SNOW-928922 send spansMixedTables flag in blob registration requests #651

Merged
merged 1 commit into from
Dec 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -18,24 +18,33 @@ class BlobMetadata {
private final Constants.BdecVersion bdecVersion;
private final List<ChunkMetadata> chunks;
private final BlobStats blobStats;
private final boolean spansMixedTables;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

want to add a comment explain this? The name is not straight forward like other fields

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or I suggest we call it something like hasMultipleTables

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment.


// used for testing only
@VisibleForTesting
BlobMetadata(String path, String md5, List<ChunkMetadata> chunks, BlobStats blobStats) {
this(path, md5, ParameterProvider.BLOB_FORMAT_VERSION_DEFAULT, chunks, blobStats);
this(
path,
md5,
ParameterProvider.BLOB_FORMAT_VERSION_DEFAULT,
chunks,
blobStats,
chunks == null ? false : chunks.size() > 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can chunks be null here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because that constructor is used in tests (only) and RegisterServiceTest calls it with chunks null.

}

BlobMetadata(
String path,
String md5,
Constants.BdecVersion bdecVersion,
List<ChunkMetadata> chunks,
BlobStats blobStats) {
BlobStats blobStats,
boolean spansMixedTables) {
this.path = path;
this.md5 = md5;
this.bdecVersion = bdecVersion;
this.chunks = chunks;
this.blobStats = blobStats;
this.spansMixedTables = spansMixedTables;
}

@JsonIgnore
Expand Down Expand Up @@ -68,13 +77,19 @@ BlobStats getBlobStats() {
return this.blobStats;
}

@JsonProperty("spans_mixed_tables")
boolean getSpansMixedTables() {
return this.spansMixedTables;
}

/** Create {@link BlobMetadata}. */
static BlobMetadata createBlobMetadata(
String path,
String md5,
Constants.BdecVersion bdecVersion,
List<ChunkMetadata> chunks,
BlobStats blobStats) {
return new BlobMetadata(path, md5, bdecVersion, chunks, blobStats);
BlobStats blobStats,
boolean spansMixedTables) {
return new BlobMetadata(path, md5, bdecVersion, chunks, blobStats, spansMixedTables);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -568,8 +568,15 @@ BlobMetadata upload(
blob.length,
System.currentTimeMillis() - startTime);

// at this point we know for sure if the BDEC file has data for more than one chunk, i.e.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
// at this point we know for sure if the BDEC file has data for more than one chunk, i.e.
// At this point we know for sure if the BDEC file has data for more than one chunk, i.e.

// spans mixed tables or not
return BlobMetadata.createBlobMetadata(
blobPath, BlobBuilder.computeMD5(blob), bdecVersion, metadata, blobStats);
blobPath,
BlobBuilder.computeMD5(blob),
bdecVersion,
metadata,
blobStats,
metadata == null ? false : metadata.size() > 1);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -719,7 +719,10 @@ List<BlobMetadata> getRetryBlobs(
blobMetadata.getMD5(),
blobMetadata.getVersion(),
relevantChunks,
blobMetadata.getBlobStats()));
blobMetadata.getBlobStats(),
// Important to not change the spansMixedTables value in case of retries. The
// correct value is the value that the already uploaded blob has.
blobMetadata.getSpansMixedTables()));
}
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import java.util.Collections;
import java.util.List;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import net.snowflake.ingest.utils.Pair;
Expand All @@ -17,7 +18,7 @@
public class RegisterServiceTest {

@Test
public void testRegisterService() {
public void testRegisterService() throws ExecutionException, InterruptedException {
RegisterService<StubChunkData> rs = new RegisterService<>(null, true);

Pair<FlushService.BlobData<StubChunkData>, CompletableFuture<BlobMetadata>> blobFuture =
Expand All @@ -26,6 +27,7 @@ public void testRegisterService() {
CompletableFuture.completedFuture(new BlobMetadata("path", "md5", null, null)));
rs.addBlobs(Collections.singletonList(blobFuture));
Assert.assertEquals(1, rs.getBlobsList().size());
Assert.assertEquals(false, blobFuture.getValue().get().getSpansMixedTables());
List<FlushService.BlobData<StubChunkData>> errorBlobs = rs.registerBlobs(null);
Assert.assertEquals(0, rs.getBlobsList().size());
Assert.assertEquals(0, errorBlobs.size());
Expand Down
Loading