Final V1 Tweaks#98
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A few rough edges in the CLI's JSON input handling ahead of V1:
pc index record searchhad an implicit default of 10 for--top-k, which required awkward internal bookkeeping (topKExplicit, a second-pass JSON probe) to correctly distinguish "flag not passed" from "flag passed as zero" when merging--bodyinput with per-field flags.--fileand--bodypointing at the same value, with--fileas the primary — despite the flag accepting inline JSON and stdin in addition to file paths.--sparse-indicesinpc index vector queryandpc index vector updatedescribed itself as an "inline JSON array" without specifying the element type, unlikepc index record searchwhich already documentedint32.Solution
pc index record search: Remove the default from--top-k(now0, marked required in help). Since0uniformly means "not provided," the body merge no longer needs explicit/default tracking —req.Query.TopK == 0is sufficient. Body parsing switches fromargio.ReadAll+ manualjson.NewDecodertoargio.DecodeJSONArg[pinecone.SearchRecordsRequest], consistent with howpc index vector queryhandles--body.Record and vector upsert: Promote
--bodyas the documented primary flag.--fileis retained as a hidden alias for backwards compatibility and will be removed after V1.pc index vector queryandpc index vector update: Clarify--sparse-indicesflag description to specifyuint32 array, making the element type visible and consistent with theint32 arrayalready documented inpc index record search.Type of Change
Test Plan
just test-unitpassespc index record searchwithout--top-kor a--bodycontainingtop_kreturns a validation errorpc index record search --top-k 5 ...andpc index record search --body '{"query":{"top_k":5,...}}'both work as beforepc index record upsert --fileandpc index vector upsert --filestill work (hidden alias)pc index vector query --sparse-indicesandpc index vector update --sparse-indiceshelp text showsuint32 array