Conversation
| * @return a future completing with the retained top-K elements, sorted from best to worst | ||
| */ | ||
| @Nonnull | ||
| public CompletableFuture<List<T>> collect(final AsyncIterable<T> iterable, |
There was a problem hiding this comment.
this method, and collectRemaining do not seem to be used anywhere. Can we remove them?
There was a problem hiding this comment.
it's a pattern used throughout MoreAsyncUtil. I am using it here as well to provide the helpers. I actually had a need for them temporarily during the development of this PR. Since TopK and DistinctTopK has been moved to async/common for general usage I keep these methods here as well. I will, however, provide proper testing, so these methods are not actually technically unused.
| * copy and its replicated copies) and provides a stable secondary ordering. Instances order by primary key first, | ||
| * then by UUID. | ||
| */ | ||
| class VectorId implements Comparable<VectorId> { |
There was a problem hiding this comment.
This looks like a textbook record to me, can you convert it to a record?
| } | ||
|
|
||
| @Override | ||
| public boolean equals(final Object o) { |
There was a problem hiding this comment.
this takes additionalValues into account, at the same time, this inherits Comparable indirectly from VectorId, including the implementation of compareTo of the parent which does not even know about additioalValues, which breaks Comparable contract.
https://docs.oracle.com/en/java/javase/25/docs/api/java.base/java/lang/Comparable.html
"The natural ordering for a class C is said to be consistent with equals if and only if e1.compareTo(e2) == 0 has the same boolean value as e1.equals(e2) for every e1 and e2 of class C."
I think this might be fine, but definitely worth some documentation.
There was a problem hiding this comment.
Please take a look at how this is solved now -- I changed the scheme to prefer composition over inheritance. The problem went away. The whole thing looks cleaner now.
|
|
||
| return primitives.fetchClusterMetadata(transaction, getTargetClusterId()) | ||
| .thenCompose(clusterMetadata -> { | ||
| if (clusterMetadata == null) { |
There was a problem hiding this comment.
same as reassign task, does this warrant a logger warning?
There was a problem hiding this comment.
We could warn on it but it's something that can occur naturally. The cluster might have gotten merged in the meantime (between enqueueing the task and executing it). In that case this can happen and is benign.
| } | ||
|
|
||
| final EnumSet<ClusterMetadata.State> states = clusterMetadata.states(); | ||
| if (!states.contains(ClusterMetadata.State.COLLAPSE)) { |
There was a problem hiding this comment.
is it possible (and ok) to have a state that is, for example, COLLAPSE + SPLIT or COLLAPSE + REASSIGN?
There was a problem hiding this comment.
ReassignTask and SplitMergeTask both will both check before they do any mutations and abort in the presence of COLLAPSE. While splitting if we decide to attempt a collapse we actually enqueue COLLAPSE and BOUNCE[SPLIT] and leave the state in COLLAPSE only. If CollapseTask encounters COLLAPSE is sufficient as it will enqueue a SPLIT afterwards anyway meaning that if something else was already waiting to do a task on something that also is in the state of COLLAPSE, it will just get clobbered and that is ok.
| // If this is a replica we may as well subsample it like reassign would normally do it. | ||
| // Collapsing a cluster should also work in an identical way if we didn't do that here. | ||
| // | ||
| replicatedTopK.add(vectorReference); |
There was a problem hiding this comment.
It feels like this is not related, or at least orthogonal, to collapse task logic?
IIUC this causes lower priority replicas to be removed until the number of replicas is less than specific threshold (replicatedClusterTarget?).
If I am right, this seems like a cleanup task that should have its own identity instead of piggypacking on collapse here? If you feel this is justified, could you please add tis behavior to the documentation of CollapseTask?
There was a problem hiding this comment.
I am doing this here because I have all the info needed, and reading the vectors is somewhat expensive. I will document it.
| // | ||
| final ImmutableSetMultimap<UUID, UUID> vectorReferenceToVectorSignatureMap = | ||
| vectorReferenceToSignatureMap(vectorReferences); | ||
| final ImmutableSetMultimap<UUID, UUID> vectorSignatureToVectorUuidMap = |
There was a problem hiding this comment.
this is a multimap holding all vectors sharing the same signature, so they are .... identical and collapsible?
There was a problem hiding this comment.
They use the same signature (the same vector), they are collapsible.
…es config) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alecgrieser
left a comment
There was a problem hiding this comment.
By file, I'm only about 1/4 of the way through this, but I figured I'd give a preliminary round first. Overall, I think this makes sense, and most of this is about documentation or clarification.
| public List<T> toSortedList() { | ||
| final PriorityQueue<T> copy = new PriorityQueue<>(queue); | ||
| final int size = copy.size(); | ||
| final T[] array = (T[]) new Object[size]; |
There was a problem hiding this comment.
Given that you then copy this into an ImmutableList anyway, I think you could alternatively use ImmutableList.newBuilderWithExpectedSize() and then return builder.build().reverse(), which returns an in-place reversed view of the list rather than making a copy
| if (assignedVectorReference.isPrimaryCopy() != vectorReference.isPrimaryCopy() || | ||
| assignedVectorReference.isUnderreplicated() != vectorReference.isUnderreplicated()) { |
There was a problem hiding this comment.
Does it need to be rewritten if the replicationPriority changes? Should this just be turned into a .equals check on the two objects?
There was a problem hiding this comment.
Yes, it should incorporate replicationPriority as suggested, I added that (also to mask out noise from SIMD operations it has to be in a range and not just bit-equal), however, we should not do a general equals() here as we dont want to also compare the vector and that equality is already guaranteed when we get here.
| * stable post-quiescence shape to inject into),</li> | ||
| * <li>find the cluster a fixed query vector would naturally belong to, using the exact | ||
| * neighborhood-discovery logic {@code Insert} uses (stable across reseeds — cluster UUIDs | ||
| * change with every split, but the "nearest cluster to vector v" choice does not), and</li> |
There was a problem hiding this comment.
Was this line deliberately removed? I think the list items don't parse any more without it
alecgrieser
left a comment
There was a problem hiding this comment.
Another additional set of comments. I'm now about 1/3 of the way through. Sorry if some of these have become outdated since I left them; I haven't gone through all of them to validate that the latest set of concurrent changes didn't make them obsolete
| final int numInnerNeighborhood, | ||
| final int numOuterNeighborhood) { |
There was a problem hiding this comment.
There's some singular/plural confusion in both the comments and also variable names in this method. I think both the inner and outer neighborhood lists can contain multiple elements, so these should probably all be "neighborhoods"
There was a problem hiding this comment.
did a big renaming, please check.
| if (foundPrimaryCluster) { | ||
| final int cappedNumInnerNeighborhood = Math.min(numInnerNeighborhood, clusterMetadataWithDistances.size()); | ||
| return new Neighborhoods(clusterMetadataWithDistances.subList(0, cappedNumInnerNeighborhood), | ||
| clusterMetadataWithDistances.subList(cappedNumInnerNeighborhood, clusterMetadataWithDistances.size())); |
There was a problem hiding this comment.
Why does this return the whole tail as the outer neighborhoods instead of incorporating the numOuterNeighborhoods list? Is it assuming that that limit is baked into the input?
There was a problem hiding this comment.
This is indeed an oversight which didn't actually matter as every caller implicitly or explicitly capped it already. In order to depend on that in the future, I capped it here though.
| * enqueued | ||
| * @param underreplicatedPrimaryClusterMax maximum number of under-replicated primary vectors in a cluster, overflow | ||
| * will result in a reassign task to be enqueued | ||
| * @param replicatedClusterMaxWrites maximum number of writes of replicated vectors to a cluster |
There was a problem hiding this comment.
"writes" here is an interesting clarification. Is this supposed to be the maximum number of replicated vectors in a cluster, or is this a limit on the amount of I/O (implied by "writes"--like per transaction)? Or is this the maximum number of clusters that a single vector will be written to (as opposed to vectors per cluster)?
| * @param replicationStatsMinSampleSize minimum number of primary vectors in a cluster before its distance statistics | ||
| * (mean/standard deviation) are trusted for the replication priority z-score term |
There was a problem hiding this comment.
What's the "trust" here about? The actual stats are accurate, right, so is this more about drift or about small sets have artificially high variance or something?
| } | ||
|
|
||
| @Nonnull | ||
| public ConfigBuilder setMetric(@Nonnull final Metric metric) { |
There was a problem hiding this comment.
minor: We're a bit inconsistent on this in our codebase, but it may be good practice to label these setters as @CanIgnoreReturnValue
| } | ||
| final long hi = readLongBigEndian(keyAsBytes, 0); | ||
| final long lo = readLongBigEndian(keyAsBytes, 8); | ||
| return new UUID(hi, lo); |
There was a problem hiding this comment.
I get that in some level, we're just using this as a signature of 128 bits in a convenient form, but should this clear out the appropriate bits so that it conforms with UUID type v4? I guess that takes us down to 126 bits of entropy, but it may avoid confusion with other types of UUIDs
| return new ClusterReference(valueTuple.getUUID(0), | ||
| storageTransform.transform(StorageHelpers.vectorFromBytes(config, valueTuple.getBytes(1)))); |
There was a problem hiding this comment.
I think I left a similar comment on one of the HNSW PRs, but it is a bit of shame that using Tuples is the path of least resistance here, rather than, say Protobuf, for all of the things that don't need to be in FDB keys (and therefore we don't care about sacrificing space for maintaining serialization order). I think the easiest path might be to split out this (and/or the HNSW) its own subproject which could also depend on Protobuf. We could theoretically change that later, though not too much later, as once we start serializing stuff in real indexes, this becomes effectively impossible to change
There was a problem hiding this comment.
Understood. I was talking to @ScottDugas the other day about moving all vector-related stuff into its own subcomponent. Then we could also do that.
| return Tuple.from(vectorId.uuid()); | ||
| } | ||
|
|
||
| static double replicationPriority(@Nonnull final Config config, |
There was a problem hiding this comment.
I'm trying to understand what this value is actually encapsulating. It's something like a weighted average of the number of standard deviations from the centroid as well as the actual distance itself. It's a bit of an odd metric, as it's combining data from two different unit systems. Is this one of those things that's taken out of a paper?
It might be good to add a bit more docs on the "why" here, or at least a reference to where this comes from
| static boolean isOccluded(@Nonnull final DistanceEstimator estimator, | ||
| @Nonnull final ClusterMetadataWithDistance replicationCandidate, | ||
| @Nonnull final List<ClusterMetadataWithDistance> selectedReplicationClusters) { |
There was a problem hiding this comment.
This could probably use Javadoc. From the implementation, I take it it's trying to figure out if for a given vector v that is being replicated into a cluster c if there's another cluster c′ such that |v - c| > |c - c′|--that is, the two clusters are closer than the vector is to the cluster it's trying to replicate into. It's not immediately obvious to me why that would automatically mean that we want to reject c or c′, though I could see why from the triangle inequality, we'd only want to pick the closest cluster to v
GuardiANN
Guided Updatable cluster Assignment and Routing via Distance-based Indexing for ANN searches.
Summary
Introduces GuardiANN, a new approximate nearest neighbor (ANN) vector structure built on FoundationDB.
GuardiANN partitions vectors into clusters with centroids indexed in an HNSW graph, supporting transactional insert, search, and delete operations with lazy background maintenance.
Architecture
Vectors are organized into clusters. Each cluster maintains:
(mean, variance, max distance to centroid) tracked via RunningStats (Welford's online algorithm)
scored by a replication priority metric
with individual IDs stored in a separate subspace
Cluster centroids are indexed in an HNSW graph for fast nearest-cluster lookup. Cluster metadata tracks vector counts, replication state, and running distance statistics.
Operations
samples vectors for RaBitQ centroid computation
Maintenance Tasks
Structural maintenance runs lazily, piggy-backed on insert/delete transactions:
underreplicated primaries and cleaning up excess replicas
itself until all dependents complete)
Configuration
All algorithm parameters are configurable via Config (record class with builder):
merge neighborhood sizes, k-means iterations/restarts, reassign neighborhood sizes, collapse duplicate threshold
parameters passed per call, not stored in config
Other Changes
after remove/subtract), converted to record
toSortedList() in DistinctTopK
Test Plan