Skip to content

fix(vchordg): avoid panic when validating an empty alpha option#468

Open
ameyypawar wants to merge 1 commit into
supervc-stack:mainfrom
ameyypawar:fix/vchordg-validate-alpha-empty-panic
Open

fix(vchordg): avoid panic when validating an empty alpha option#468
ameyypawar wants to merge 1 commit into
supervc-stack:mainfrom
ameyypawar:fix/vchordg-validate-alpha-empty-panic

Conversation

@ameyypawar

Copy link
Copy Markdown

What

VchordgIndexOptions::validate_alpha indexes alpha[0] after the is_sorted() and range checks — both of which are vacuously true for an empty slice:

fn validate_alpha(alpha: &[f32]) -> Result<(), ValidationError> {
    if !alpha.is_sorted() { ... }                            // [].is_sorted() == true  -> skip
    if !alpha.iter().all(|x| (1.0..2.0).contains(x)) { ... } // [].iter().all(..) == true -> skip
    if alpha[0] != 1.0 { ... }                               // [] -> index out of bounds
    Ok(())
}

So an empty alpha panics (index out of bounds) instead of returning a ValidationError. It's reachable from SQL via WITH (options = '[index] alpha = []'), which surfaces a cryptic panic message rather than a clean validation error.

Fix

Use alpha.first() so the empty case yields the existing `alpha` should contain `1.0` error:

if alpha.first() != Some(&1.0) {
    return Err(ValidationError::new("`alpha` should contain `1.0`"));
}

For any non-empty input alpha.first() == Some(&alpha[0]), so behavior is unchanged; only the empty slice goes from panic to a proper validation error.

Tests

Adds unit tests for validate_alpha covering the empty slice (regression) plus valid/invalid cases. These run under the existing cargo test --workspace --exclude vchord job (vchordg is a pure-Rust crate).

Happy to adjust the wording or drop the tests if you'd prefer a more minimal change.

`validate_alpha` indexes `alpha[0]` after the sorted/range checks, both of
which pass on an empty slice (`[].is_sorted()` and `[].iter().all(..)` are
true). So an empty `alpha` (e.g. `WITH (options='[index] alpha = []')`)
panics with an index-out-of-bounds instead of returning a ValidationError.

Use `alpha.first()` so the empty case yields the existing "`alpha` should
contain `1.0`" error. Behavior is unchanged for every non-empty input.

Add unit tests covering the empty slice plus valid/invalid cases.
@github-actions

Copy link
Copy Markdown


Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

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.

1 participant