Skip to content
Open
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
@@ -0,0 +1,76 @@
/*
* DataInKeySpacePath.java
*
* This source file is part of the FoundationDB open source project
*
* Copyright 2015-2025 Apple Inc. and the FoundationDB project authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.apple.foundationdb.record.provider.foundationdb.keyspace;

import com.apple.foundationdb.KeyValue;
import com.apple.foundationdb.record.provider.foundationdb.FDBRecordContext;
import com.apple.foundationdb.tuple.Tuple;
import com.apple.foundationdb.tuple.TupleHelpers;

import java.util.concurrent.CompletableFuture;

/**
* Class representing a {@link KeyValue} pair within in {@link KeySpacePath}.
*/
public class DataInKeySpacePath {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this implement equals and hashCode?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, probably, but I'm not entirely sure how to do that with the CompletableFuture for the resolved path.
It can either fall through and say it's equal if the original path, and raw value or equal, or, alternately, I can change it so that the constructor takes the resolved path.

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 started doing this path, an discovered that ResolvedKeySpacePath.equals is broken, and I've started working on a fix for that. Its a fair amount of new tests, and a couple other equals/hashcode implementations, so I'm going to pull that out into a separate PR, and come back to this once it is fixed.

Issue: #3594


final CompletableFuture<ResolvedKeySpacePath> resolvedPath;
final KeyValue rawKeyValue;

public DataInKeySpacePath(KeySpacePath path, KeyValue rawKeyValue, FDBRecordContext context) {
this.rawKeyValue = rawKeyValue;

// Convert the raw key to a Tuple and resolve it starting from the provided path
Tuple keyTuple = Tuple.fromBytes(rawKeyValue.getKey());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to be concerned with keys that are not Tuple parseable? I believe all of the keys that we currently generate are Tuple parseable, though we could adjust that in the future

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

KeySpacePathImpl always produces tuples, and AFAIK every key we have ever used has been in tuples. I feel like committing to that at this point, is worthwhile. If we find a strong reason to put non-tuple data in a key, we'll have to cross that bridge when we get to it.


// First resolve the provided path to get its resolved form
this.resolvedPath = path.toResolvedPathAsync(context).thenCompose(resolvedPath -> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It almost seems like the method provided should be a method on the KeySpacePath. I was sort of surprised that we didn't already have it. I can see how we'd have to be clear in the name that we're starting with the full key rather than just the suffix. I suppose we could structure this here to go the other way: we resolve the KeySpacePath first, use it to get a raw byte prefix, and then resolve the Tuple suffix using findChildForKey

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I was also kind of surprised. I was a bit fixated on getting this working, that I didn't pause to think that I should add it there. I'll move most of this there.

// Now use the resolved path to find the child for the key
// We need to figure out how much of the key corresponds to the resolved path
Tuple pathTuple = resolvedPath.toTuple();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this validate that pathTuple is a prefix of keyTuple? Strictly speaking, I don't think the check is necessary, but it could be a sensible thing to validate

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I think that's a good double check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: I took advantage of TupleHelpers.isPrefix, which apparently didn't have any unit tests, but I'm adding them in a followup PR. https://github.com/FoundationDB/fdb-record-layer/pull/3578/files#diff-c7f3ee5c693bba884071e40ab98f13c12a86350a819f71f1c6a60eb7b750623c
I'm not sure if it's worth bringing into this PR.

int pathLength = pathTuple.size();

// The remaining part of the key should be resolved from the resolved path's directory
if (keyTuple.size() > pathLength) {
// There's more in the key than just the path, so resolve the rest
if (resolvedPath.getDirectory().getSubdirectories().isEmpty()) {
return CompletableFuture.completedFuture(
new ResolvedKeySpacePath(resolvedPath.getParent(), resolvedPath.toPath(),
resolvedPath.getResolvedPathValue(),
TupleHelpers.subTuple(keyTuple, pathTuple.size(), keyTuple.size())));
} else {
return resolvedPath.getDirectory().findChildForKey(context, resolvedPath, keyTuple, keyTuple.size(), pathLength);
}
} else {
// The key exactly matches the path
return CompletableFuture.completedFuture(resolvedPath);
}
});
}

public CompletableFuture<ResolvedKeySpacePath> getResolvedPath() {
return resolvedPath;
}

public KeyValue getRawKeyValue() {
return rawKeyValue;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -566,4 +566,20 @@ default List<ResolvedKeySpacePath> listSubdirectory(@Nonnull FDBRecordContext co
*/
@API(API.Status.UNSTABLE)
String toString(@Nonnull Tuple tuple);

/**
* Export all data stored under this KeySpacePath and return it in a RecordCursor.
* This method scans all keys that have this path as a prefix and returns the key-value pairs.
* Supports continuation to resume scanning from a previous position.
*
* @param context the transaction context in which to perform the data export
* @param continuation optional continuation from a previous export operation, or null to start from the beginning
* @param scanProperties properties controlling how the scan should be performed
* @return a RecordCursor that iterates over all KeyValue pairs under this path
*/
@API(API.Status.UNSTABLE)
@Nonnull
RecordCursor<DataInKeySpacePath> exportAllData(@Nonnull FDBRecordContext context,
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 realized that this is an interface, and while I don't think we intended anyone to extend it, I should probably put a default implementation in.

@Nullable byte[] continuation,
@Nonnull ScanProperties scanProperties);
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,14 @@
import com.apple.foundationdb.record.RecordCursor;
import com.apple.foundationdb.record.ScanProperties;
import com.apple.foundationdb.record.ValueRange;
import com.apple.foundationdb.record.cursors.LazyCursor;
import com.apple.foundationdb.record.provider.foundationdb.FDBRecordContext;
import com.apple.foundationdb.record.provider.foundationdb.KeyValueCursor;
import com.apple.foundationdb.subspace.Subspace;
import com.apple.foundationdb.tuple.ByteArrayUtil;
import com.apple.foundationdb.tuple.Tuple;
import com.google.common.collect.Lists;

import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import java.util.ArrayList;
Expand Down Expand Up @@ -337,6 +341,21 @@ public String toString() {
return toString(null);
}

@Nonnull
@Override
public RecordCursor<DataInKeySpacePath> exportAllData(@Nonnull FDBRecordContext context,
@Nullable byte[] continuation,
@Nonnull ScanProperties scanProperties) {
return new LazyCursor<>(toTupleAsync(context)
.thenApply(tuple -> KeyValueCursor.Builder.withSubspace(new Subspace(tuple))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to try and head off some potential problems: in #3397, there's a modification being made to the KeyValueCursor continuation to wrap it in a protobuf. This would allow us to avoid returning an empty ByteString as the continuation in case there is a single key range, which may be something that a KeySpacePath could run into. (It would require that the first key in the keyspace path is set, I think.)

There's sort of an open question as to how we migrate all of the uses over from the old format to the new format. It would be nice if we could set the serialization mode on this one to the new format so that we don't have to worry about migrating it later, I think. Though that would introduce a dependency on #3397 being merged first.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good point.
I changed exportAllData on the interface to be EXPERIMENTAL, and added a note that we expect the continuation change without preserving backwards compatibility when that merges. If the rest of the pieces are in place such that this is useful before #3397 gets merged, we can potentially update the javadoc, and make this be backwards compatible.

.setContext(context)
.setContinuation(continuation)
.setScanProperties(scanProperties)
.build()),
context.getExecutor())
.map(keyValue -> new DataInKeySpacePath(this, keyValue, context));
}

/**
* Returns this path properly wrapped in whatever implementation the directory the path is contained in dictates.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -226,4 +226,12 @@ public String toString() {
public String toString(@Nonnull Tuple t) {
return inner.toString(t);
}

@Nonnull
@Override
public RecordCursor<DataInKeySpacePath> exportAllData(@Nonnull FDBRecordContext context,
@Nullable byte[] continuation,
@Nonnull ScanProperties scanProperties) {
return inner.exportAllData(context, continuation, scanProperties);
}
}
Loading
Loading