cockroach icon indicating copy to clipboard operation
cockroach copied to clipboard

fix pulling full vectors from the primary key when column families are in use

Open mw5h opened this issue 6 months ago • 7 comments

vecindex: delete Manager.GetWithDesc() method

This method was added because backfilling indexes are not considered deletable. In retrospect, there's no reason why we need to restrict an internal interface this way and deleting it removes a fair bit of complexity from upcoming changes to the vector store.

Informs: #146046 Release note: None

fetcher: return spanID from NextRowDecoded()

Previously, Fetcher.NextRowDecoded() discarded the spanID it got from Fetcher.NextRow(). This information is needed for a future use of NextRowDecoded(), so this patch passes it through. Existing users ignore this return value.

Informs: #146046 Release note: None

vecstore: use span.Builder in implementation of GetFullVectors

Previously, the vector store stored the table and index descriptors for the vector index it was associated with. Since the store itself did not have a lease on this descriptor and would outlive the transaction that caused the store to be created, we could end up using a stale table descriptor for vector operations.

This patch moves table information into the vector transaction in the form of an index fetch spec created by a leaseholder on the table. The vecindex transaction is expected to have a lifetime shorter than the lease held by the caller, so the descriptors that generate this fetch spec will not go stale.

Addtionally, we generate the needed family IDs for a span.Splitter(), to avoid reading families that we don't require. This is also included with the fetch spec.

This patch also:

  • Introduces a proto buffer to hold the index fetch spec and family IDs.
  • Introduces a function to initialize this proto buffer.
  • Introduces a testing API into the vecstore so that tests can provide fake family descriptors to vecstore.

Fixes: #146046 Release note (bug fix): A bug where using column families on tables with vector indexes would cause the index to fail to return results has been fixed.

vecstore: correctly handle composite pk columns

Previously, the vector store would blindly use primary key column bytes encoded in a vector index key to rebuild the primary key when retrieving full vector values. SpanBuilder will attempt to reused already encoded datums when building a span, but it can't do so if the encoded datum was encoded in a different direction than required for the span being built. For non-composite columns this was not an issue because they have all the data needed to re-encode in the key value itself. For composite column, the data encoded in the key is a key analogue and can't be used to reconstruct the original column value, which is stored as a suffix column.

For vector search, the search processor has access to the ValueBytes array as well as the key, so it's able to rebuild column values as needed. However, the fixup processor only has access to the key bytes, which requires a bit of trickery.

In this patch, we disallow assigning directionality to vector index key columns. The mechanics of similarity search preclude scannign prefixes, so this was always a nonsensical thing to do. We now disallow it entirely and take the further step to mechanically switch the directionality of vector index prefix columns to match the primary key, ensuring that the encoded value of these shared columns is always directly usable.

We also move the logic for decoding and rebuilding the primary key into the vector store so that it can be shared between fixups and the vector search processor. Note that there is an additional composite key decoder available for the vector search processor because it does indeed need to decode the primary key bytes back to column values.

Along the way, we:

  • Remove ASC/DESC from vector index columns.
  • Introduce a new index type method to describe indexes with non-directional prefixes.
  • Alter SHOW commands to no longer show vector index column directionality.
  • Introduce a bunch of test-gated code to ensure everything I said above is in fact true and remains so in the future.

Informs: #146046 Release note (sql change): It is no longer permitted to assign directionality to any vector index column. Prefix columns (those columns before the vector column in a compound vector index key) are not scannable in a vector index, so directionality isn't relevant to them.

mw5h avatar May 26 '25 22:05 mw5h

This change is Reviewable

cockroach-teamcity avatar May 26 '25 22:05 cockroach-teamcity

Okay, I'll wait for @DrewKimball to look at the span.Builder & span.Splitter stuff.

mw5h avatar May 28 '25 18:05 mw5h

RFAL

mw5h avatar Jun 06 '25 19:06 mw5h

I'm not sure I follow your last question. I think the only real problem left here is the prefix column doesn't match PK column direction, which I'm working on. My plan is to disallow explicit direction modifiers in vector indexes and then apply the directions from the PK for any shared columns.

mw5h avatar Jun 09 '25 16:06 mw5h

I'm not sure I follow your last question. I think the only real problem left here is the prefix column doesn't match PK column direction, which I'm working on. My plan is to disallow explicit direction modifiers in vector indexes and then apply the directions from the PK for any shared columns.

If you mean this question:

Are you leaning toward finishing up this approach, or pulling apart TreeKey and KeyBytes?

Then what I'm asking is, do you plan on sticking with SpanBuilder + splitter and relying on the code path that avoids decoding the EncDatums, or will you end up "manually" splitting apart TreeKey and KeyBytes and recombining into a valid primary key?

DrewKimball avatar Jun 09 '25 16:06 DrewKimball

I'm really not excited to introduce another SpanBuilder-like thing that isn't SpanBuilder, so I do want to rely upon having the encoded datums cached. I guess it would make sense to ensure that, should the encoded data not be present, we emit some sort of error instead of plowing ahead.

mw5h avatar Jun 09 '25 18:06 mw5h

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

:owl: Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

blathers-crl[bot] avatar Jun 10 '25 00:06 blathers-crl[bot]

bors r+

mw5h avatar Jun 21 '25 00:06 mw5h

Build failed (retrying...):

craig[bot] avatar Jun 21 '25 01:06 craig[bot]