Simplify CLI commands and fix query regressions#15
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a modular CLI (cli/command/parsing) and moves command handling out of main; refactors segment search candidate-sizing and top-k/IVF nprobe logic (introducing Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "CLI Parser\n(clap)"
participant Cmd as "Command Dispatcher\n(run_command)"
participant Parser as "Parsing Utils\n(read_jsonl/parse_index_params)"
participant Engine as "Engine / Database"
User->>CLI: provide argv
CLI->>CLI: parse into `Cli` / `Command`
CLI-->>Cmd: selected `Command`
Cmd->>Engine: open database (root)
Cmd->>Parser: parse inputs (JSONL, vector, index params)
Parser-->>Cmd: typed args / JSON docs
Cmd->>Engine: execute operation (create/insert/query/index/DDL)
Engine-->>Cmd: result / status
Cmd->>Parser: serialize output to JSON
Parser-->>User: print JSON / status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (8)
crates/garuda-segment/src/search.rs (1)
202-209: Consider renaming parameterindexed_doc_count→candidate_doc_countfor consistency.The
search_ivf_hitsfunction still uses the parameter nameindexed_doc_count(line 207), but callers now passstats.candidate_doc_count(line 180). The semantics have changed—this value now represents total records including deleted ones. Aligning the parameter name would improve clarity for future maintainers.Same applies to
ann_candidate_top_kat line 298.♻️ Suggested rename
fn search_ivf_hits( segment: SearchSegment<'_>, query_vector: &garuda_types::DenseVector, recall: garuda_types::IvfRecallPlan, candidate_top_k: TopK, - indexed_doc_count: usize, + candidate_doc_count: usize, visible_doc_count: usize, allowed_visible_doc_count: usize, ) -> Result<Vec<IvfSearchHit>, Status> {fn ann_candidate_top_k( top_k: TopK, budget: AnnBudgetPolicy, filter: SegmentFilterContext<'_>, - indexed_doc_count: usize, + candidate_doc_count: usize, visible_doc_count: usize, ) -> TopK {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/garuda-segment/src/search.rs` around lines 202 - 209, Rename the misleading parameter `indexed_doc_count` to `candidate_doc_count` in the function signature for `search_ivf_hits` and update all internal uses within that function to reflect the new name, since callers now pass `stats.candidate_doc_count`; likewise rename the `ann_candidate_top_k` parameter/variable usage (the one referenced at line ~298) to match `candidate_top_k`/`ann_candidate_top_k` naming consistency, and update any callers or references to use the new identifier so semantics (total records including deleted ones) are clear and consistent across `search_ivf_hits` and ANN candidate handling.garuda/src/cli.rs (1)
136-150: Consider usingNonZeroUsizefortop_k.
top_kis defined asusize, allowing 0, butTopK::newincommand.rswill reject it at runtime. UsingNonZeroUsizewould provide earlier validation at the CLI parsing stage.Suggested change
#[derive(Args)] pub struct QueryArgs { #[arg(long)] pub value: String, #[arg(long)] - pub top_k: usize, + pub top_k: NonZeroUsize,This would require updating
command.rsto usetop_k.get()when callingTopK::new.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@garuda/src/cli.rs` around lines 136 - 150, Change the QueryArgs struct's top_k field from usize to std::num::NonZeroUsize so the CLI parser rejects 0 values early; update any call sites in command.rs that construct TopK (e.g., TopK::new(...)) to call .get() on top_k (TopK::new(top_k.get())) and adjust types/parameter names accordingly wherever QueryArgs::top_k is used to compile with the NonZeroUsize change.garuda/src/command.rs (3)
27-34: RedundantCollectionOptions::default()calls.Three separate
CollectionOptions::default()calls are made. Consider caching the default once.Suggested simplification
} => { + let default_options = CollectionOptions::default(); let options = CollectionOptions { segment_max_docs: segment_max_docs - .unwrap_or(CollectionOptions::default().segment_max_docs), + .unwrap_or(default_options.segment_max_docs), storage_access: storage_access .map(Into::into) - .unwrap_or(CollectionOptions::default().storage_access), - ..CollectionOptions::default() + .unwrap_or(default_options.storage_access), + ..default_options };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@garuda/src/command.rs` around lines 27 - 34, Cache a single CollectionOptions::default() and reuse it instead of calling it three times: create let default = CollectionOptions::default(); then build options with CollectionOptions { segment_max_docs: segment_max_docs.unwrap_or(default.segment_max_docs), storage_access: storage_access.map(Into::into).unwrap_or(default.storage_access), ..default } so you reference the default fields only once; update the code around the options variable accordingly.
17-19: UnreachableCommand::Initarm.The
Initvariant is already handled inmain.rsbefore callingrun_command, making this arm unreachable. Consider removing it or adding anunreachable!()macro to document the invariant.Suggested change
pub fn run_command(db: &Database, command: Command) -> Result<(), String> { match command { - Command::Init => Ok(()), + Command::Init => unreachable!("Init is handled in main.rs"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@garuda/src/command.rs` around lines 17 - 19, The match arm handling Command::Init inside run_command is unreachable because main.rs handles Init before calling run_command; remove the Command::Init arm or replace its body with unreachable!() to document the invariant. Locate the pattern match in the run_command function and either delete the Command::Init => Ok(()) arm entirely or change it to Command::Init => unreachable!() so the code and intent are explicit.
184-192: Use ofstd::mem::takefor value extraction is correct but could be cleaner.The
std::mem::takeonargs.valueworks, but sinceargsis passed by value and consumed, you could destructure it instead for clarity.Alternative approach
-fn id_query(mut args: QueryArgs) -> Result<VectorQuery, String> { +fn id_query(args: QueryArgs) -> Result<VectorQuery, String> { + let QueryArgs { value, top_k, filter, include_vector, fields, search } = args; let mut query = VectorQuery::by_id( field_name(VECTOR_FIELD), - parse_doc_id(std::mem::take(&mut args.value))?, - TopK::new(args.top_k).map_err(|status| status.message)?, + parse_doc_id(value)?, + TopK::new(top_k).map_err(|status| status.message)?, ); - apply_query_args(&mut query, args)?; + query.filter = filter; + query.output_fields = fields; + query.vector_projection = if include_vector { + VectorProjection::Include + } else { + VectorProjection::Exclude + }; + query.search = parse_query_search(search)?; Ok(query) }This eliminates the need for
apply_query_argsreuse at the cost of some duplication, but if you prefer the current structure, it's also acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@garuda/src/command.rs` around lines 184 - 192, Replace the std::mem::take(&mut args.value) pattern by destructuring the consumed QueryArgs in id_query to extract the pieces you need (e.g., value and top_k) and use those bindings when building the VectorQuery (call parse_doc_id on the extracted value and TopK::new on the extracted top_k); because destructuring consumes args you will need to either inline the small bits of logic currently in apply_query_args for this case or reconstruct the remaining QueryArgs to pass into apply_query_args, and then proceed to call apply_query_args or its inlined equivalent before returning the query (refer to id_query, QueryArgs, parse_doc_id, TopK::new, and apply_query_args to find the spots to change).garuda/src/parsing.rs (1)
248-264: Potential precision loss for large unsigned integers.Numbers that don't fit in
i64(likeu64values >i64::MAX) will be parsed asf64, which can lose precision for values > 2^53. This is a known limitation of JSON number handling.If the domain requires large exact integers, consider checking
as_u64()beforeas_f64(). For most use cases, this is acceptable.Optional: Handle u64 explicitly if needed
fn parse_scalar_value(value: serde_json::Value) -> Result<ScalarValue, String> { match value { serde_json::Value::Null => Ok(ScalarValue::Null), serde_json::Value::Bool(value) => Ok(ScalarValue::Bool(value)), serde_json::Value::Number(value) => { if let Some(value) = value.as_i64() { return Ok(ScalarValue::Int64(value)); } + // Note: Large u64 values would need ScalarType::UInt64 support if let Some(value) = value.as_f64() { return Ok(ScalarValue::Float64(value)); } Err(String::from("unsupported numeric value")) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@garuda/src/parsing.rs` around lines 248 - 264, The parse_scalar_value function currently converts JSON numbers by trying as_i64 then as_f64, which causes precision loss for large unsigned integers; update parse_scalar_value to check value.as_i64() first, then value.as_u64() and map to a new or existing ScalarValue::UInt64 (or an appropriate UInt variant), and only then fallback to value.as_f64(); ensure you update the ScalarValue enum if necessary to include a UInt64 variant and adjust error handling/messages in parse_scalar_value accordingly.garuda/tests/test_cli_contract.rs (2)
388-420: Test failure reason may be misleading.The test
create_from_schema_requires_options_in_fileexpects failure due to missingoptions, but the schema also has an emptyfieldsarray while referencingprimary_key: "pk". This would fail schema validation even if options were present. Consider either:
- Renaming the test to reflect it tests general invalid schema handling, or
- Adding a valid schema with only
optionsmissing to specifically test that requirementSuggested fix for clearer test intent
#[test] fn create_from_schema_requires_options_in_file() { let tmp = temp_path("cli-invalid-schema-file"); std::fs::create_dir_all(&tmp).expect("create temp root"); let schema_path = tmp.join("schema.json"); std::fs::write( &schema_path, serde_json::json!({ "schema": { "name": "docs", "primary_key": "pk", - "fields": [], + "fields": [ + { + "name": "pk", + "field_type": "String", + "index": "None", + "nullability": "Required", + "default_value": null + } + ], "vector": { "name": "embedding", "dimension": 4, "metric": "Cosine", "indexes": "DefaultFlat" } } }) .to_string(), ) .expect("write invalid schema file");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@garuda/tests/test_cli_contract.rs` around lines 388 - 420, The test create_from_schema_requires_options_in_file is ambiguous because the provided schema has an empty "fields" array while referencing primary_key "pk", causing schema validation to fail for the wrong reason; update the test so it specifically omits only the "options" field: modify the JSON in the test to include a field entry for the primary key (e.g., an object with "name": "pk" and appropriate "type") so the schema is otherwise valid, leaving out "options", or alternatively rename the test to something like create_from_schema_rejects_invalid_schema if you intend to cover general schema validation; ensure run_cli still expects failure and keep the assert as-is.
24-26: Consider adding error context tostdout_jsonhelper.If JSON parsing fails, the current error message won't show the actual stdout content, making debugging difficult.
Suggested improvement
fn stdout_json(output: &std::process::Output) -> Value { - serde_json::from_slice(&output.stdout).expect("stdout should be valid json") + serde_json::from_slice(&output.stdout).unwrap_or_else(|e| { + panic!( + "stdout should be valid json: {e}\nstdout: {}", + String::from_utf8_lossy(&output.stdout) + ) + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@garuda/tests/test_cli_contract.rs` around lines 24 - 26, The helper stdout_json currently calls serde_json::from_slice(&output.stdout).expect("stdout should be valid json") which loses the actual stdout content on parse failure; update the stdout_json function to include the stdout bytes (decoded via String::from_utf8_lossy or similar) in the error context or expect message so failures show the JSON parse error plus the actual stdout content for debugging (refer to stdout_json and its use of output.stdout).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/garuda-segment/src/search.rs`:
- Around line 202-209: Rename the misleading parameter `indexed_doc_count` to
`candidate_doc_count` in the function signature for `search_ivf_hits` and update
all internal uses within that function to reflect the new name, since callers
now pass `stats.candidate_doc_count`; likewise rename the `ann_candidate_top_k`
parameter/variable usage (the one referenced at line ~298) to match
`candidate_top_k`/`ann_candidate_top_k` naming consistency, and update any
callers or references to use the new identifier so semantics (total records
including deleted ones) are clear and consistent across `search_ivf_hits` and
ANN candidate handling.
In `@garuda/src/cli.rs`:
- Around line 136-150: Change the QueryArgs struct's top_k field from usize to
std::num::NonZeroUsize so the CLI parser rejects 0 values early; update any call
sites in command.rs that construct TopK (e.g., TopK::new(...)) to call .get() on
top_k (TopK::new(top_k.get())) and adjust types/parameter names accordingly
wherever QueryArgs::top_k is used to compile with the NonZeroUsize change.
In `@garuda/src/command.rs`:
- Around line 27-34: Cache a single CollectionOptions::default() and reuse it
instead of calling it three times: create let default =
CollectionOptions::default(); then build options with CollectionOptions {
segment_max_docs: segment_max_docs.unwrap_or(default.segment_max_docs),
storage_access:
storage_access.map(Into::into).unwrap_or(default.storage_access), ..default } so
you reference the default fields only once; update the code around the options
variable accordingly.
- Around line 17-19: The match arm handling Command::Init inside run_command is
unreachable because main.rs handles Init before calling run_command; remove the
Command::Init arm or replace its body with unreachable!() to document the
invariant. Locate the pattern match in the run_command function and either
delete the Command::Init => Ok(()) arm entirely or change it to Command::Init =>
unreachable!() so the code and intent are explicit.
- Around line 184-192: Replace the std::mem::take(&mut args.value) pattern by
destructuring the consumed QueryArgs in id_query to extract the pieces you need
(e.g., value and top_k) and use those bindings when building the VectorQuery
(call parse_doc_id on the extracted value and TopK::new on the extracted top_k);
because destructuring consumes args you will need to either inline the small
bits of logic currently in apply_query_args for this case or reconstruct the
remaining QueryArgs to pass into apply_query_args, and then proceed to call
apply_query_args or its inlined equivalent before returning the query (refer to
id_query, QueryArgs, parse_doc_id, TopK::new, and apply_query_args to find the
spots to change).
In `@garuda/src/parsing.rs`:
- Around line 248-264: The parse_scalar_value function currently converts JSON
numbers by trying as_i64 then as_f64, which causes precision loss for large
unsigned integers; update parse_scalar_value to check value.as_i64() first, then
value.as_u64() and map to a new or existing ScalarValue::UInt64 (or an
appropriate UInt variant), and only then fallback to value.as_f64(); ensure you
update the ScalarValue enum if necessary to include a UInt64 variant and adjust
error handling/messages in parse_scalar_value accordingly.
In `@garuda/tests/test_cli_contract.rs`:
- Around line 388-420: The test create_from_schema_requires_options_in_file is
ambiguous because the provided schema has an empty "fields" array while
referencing primary_key "pk", causing schema validation to fail for the wrong
reason; update the test so it specifically omits only the "options" field:
modify the JSON in the test to include a field entry for the primary key (e.g.,
an object with "name": "pk" and appropriate "type") so the schema is otherwise
valid, leaving out "options", or alternatively rename the test to something like
create_from_schema_rejects_invalid_schema if you intend to cover general schema
validation; ensure run_cli still expects failure and keep the assert as-is.
- Around line 24-26: The helper stdout_json currently calls
serde_json::from_slice(&output.stdout).expect("stdout should be valid json")
which loses the actual stdout content on parse failure; update the stdout_json
function to include the stdout bytes (decoded via String::from_utf8_lossy or
similar) in the error context or expect message so failures show the JSON parse
error plus the actual stdout content for debugging (refer to stdout_json and its
use of output.stdout).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ae3745d-915a-4837-9187-5236967ca7f3
📒 Files selected for processing (8)
crates/garuda-engine/tests/test_collection_ordering_behavior.rscrates/garuda-segment/src/search.rscrates/garuda-segment/tests/test_ivf_sidecar.rsgaruda/src/cli.rsgaruda/src/command.rsgaruda/src/main.rsgaruda/src/parsing.rsgaruda/tests/test_cli_contract.rs
PhantomInTheWire
force-pushed
the
codex/cli-simplify-and-fix-ci
branch
from
d8502ce to
134500d
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
PhantomInTheWire
force-pushed
the
codex/cli-simplify-and-fix-ci
branch
from
8bc568d to
ef077d8
Compare
PhantomInTheWire
force-pushed
the
codex/cli-simplify-and-fix-ci
branch
from
ef077d8 to
dfa9998
Compare
PhantomInTheWire
force-pushed
the
codex/cli-simplify-and-fix-ci
branch
from
edc9e44 to
561669e
Compare
PhantomInTheWire
force-pushed
the
codex/cli-simplify-and-fix-ci
branch
from
176bdcb to
1c3df88
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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
1 participant
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.
Summary
main.rsas the orchestration entrypoint{ schema, options }shape forcreate-from-schemaTesting
cargo fmt --all --checkcargo testSummary by CodeRabbit
New Features
Improved Search
Tests