feat: support more vector item metadata value types #972
Conversation
In the previous version of the service, we only supported string-valued metadata. In the latest version, we support the following value types: - string - int - double - bool - list of strings We update the `VectorItem` and `SearchHit` interfaces to support metadata of these types (where int and float are both represented as `number`). We update `sendUpsertItemBatch` to serialize the metadata appropriately. We distinguish numbers that are integers vs floating-point. Similarly we update `sendSearch` to deserialize the metadata appropriately. We add an extra integration test to ensure each of the metadata value types are stored and retrieved properly.
| // eslint-disable-next-line @typescript-eslint/no-unsafe-call | ||
| metadata.setIntegerValue(value); |
There was a problem hiding this comment.
I am unclear why some of these modifiers have unsafe call warnings and others do not.
There was a problem hiding this comment.
I pulled this down locally to test this. I don't think that you need any of these eslint-disable lines actually. The js protobuf dependecies trip up some IDEs. I suspect that your IDE is operating off of some stale data, and that it thinks that item is of type any or unknown. I think if you remove all of the eslint-disable lines, and rebuild in your terminal it will pass. (Verified this :onmybox: ). Sometimes if you rebuild on your terminal, and then open up the project inside your IDE, it will pickup the latest changes. :skeletor_fist_shake: caching issues :skeletor_fist_shake:
malandis
requested review from
cprice404,
nand4011,
pratik151192,
anitarua,
bruuuuuuuce and
rishtigupta
|
I regenerated the |
| string_value: value, | ||
| }); | ||
| } else if (typeof value === 'number') { | ||
| if (Number.isInteger(value)) { |
| acc[metadata.getField()] = metadata.getStringValue(); | ||
| const field = metadata.getField(); | ||
| switch (metadata.getValueCase()) { | ||
| case vectorindex._Metadata.ValueCase.STRING_VALUE: |
There was a problem hiding this comment.
curious if there is a reason you are able to use constants here but you had to use string literals for the earlier case statement
There was a problem hiding this comment.
That appears to be a grpc-js vs grpc-web thing. grpc-js uses string literals and grpc-web constants. 🤸♂️
There was a problem hiding this comment.
cool. bonus points for defining our own constants for it in the near future but definitely not a blocker.
| return acc; | ||
| }, {} as Record<string, string>), | ||
| }, {} as Record<string, string | number | boolean | Array<string>>), |
There was a problem hiding this comment.
just curious if you tried a type alias for this in JS too, and observed the same issue where the IDEs didn't expand it in a helpful way
There was a problem hiding this comment.
Same thing in the sense that if we make a type alias T that is used in a function signature for foo, then if you type foo you will only see T, unless you manually click the function, go to definition, and then hover over T.
In the previous version of the service, we only supported
string-valued metadata. In the latest version, we support the
following value types:
We update the
VectorItemandSearchHitinterfaces to supportmetadata of these types (where int and float are both represented as
number).We update
sendUpsertItemBatchto serialize the metadataappropriately. We distinguish numbers that are integers vs
floating point.
Similarly we update
sendSearchto deserialize the metadataappropriately.
We add an extra integration test to ensure each of the metadata value
types are stored and retrieved properly.