Skip to content

VS-257: Fix Vectorize query options#2473

Merged
fhanau merged 1 commit intocloudflare:mainfrom
garvit-gupta:garvit/VS-257/fix-vectorize-types
Aug 16, 2024
Merged

VS-257: Fix Vectorize query options#2473
fhanau merged 1 commit intocloudflare:mainfrom
garvit-gupta:garvit/VS-257/fix-vectorize-types

Conversation

@garvit-gupta
Copy link
Copy Markdown
Contributor

@garvit-gupta garvit-gupta commented Aug 2, 2024

This change fixes 2 aspects of Vectorize query options.

  1. Options itself should be an optional field.

  2. Options.returnMetadata should be of type boolean for Vectorize V1 and of type VectorizeMetadataRetrievalLevel for Vectorize V2. The existing type relied on a generic to assign type appropriately for the operation, but did not work as expected for Vectorize V2. It continued to expect a boolean field for V2 as well instead of VectorizeMetadataRetrievalLevel.

Follows: #2443

@garvit-gupta garvit-gupta requested review from a team as code owners August 2, 2024 05:12
@garvit-gupta garvit-gupta force-pushed the garvit/VS-257/fix-vectorize-types branch from 28f21c5 to 0bfd15b Compare August 14, 2024 18:59
@garvit-gupta garvit-gupta force-pushed the garvit/VS-257/fix-vectorize-types branch 2 times, most recently from 538774b to 0bfd15b Compare August 15, 2024 16:53
delete v.values;
});
if (!body?.returnMetadata)
if (!body?.returnMetadata || body?.returnMetadata === "none")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (!body?.returnMetadata || body?.returnMetadata === "none")
if ((body?.returnMetadata || 'none') === "none")

nit: Might be a bit cleaner

@anonrig
Copy link
Copy Markdown
Contributor

anonrig commented Aug 15, 2024

@jasnell is internal pr needed for this change?

@jasnell
Copy link
Copy Markdown
Collaborator

jasnell commented Aug 15, 2024

To be safe I think we should have an internal CI run yes.

@garvit-gupta garvit-gupta force-pushed the garvit/VS-257/fix-vectorize-types branch from 0bfd15b to 13044c0 Compare August 16, 2024 17:33
@jasnell
Copy link
Copy Markdown
Collaborator

jasnell commented Aug 16, 2024

Internal CI run was goodl

@fhanau fhanau merged commit 7c3812d into cloudflare:main Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants