Skip to content

feat(kividb): filter multi-valued labels on KiviDB >= 1.0.5; CI on v1… - #326

Closed
murtazayusuf wants to merge 1 commit into
redis:masterfrom
kividbio:kividb-1.0.5
Closed

murtazayusuf wants to merge 1 commit into
redis:masterfrom
kividbio:kividb-1.0.5

Conversation

@murtazayusuf

Copy link
Copy Markdown

….0.5-full

KiviDB 1.0.5 accepts FT.CREATE TAG SEPARATOR and splits stored values on it, so the multi-valued labels field no longer needs to be a hard error.

  • New kividb_caps library module (unit-tested): parses kividb_version from INFO server and gates multi-valued TAG support at >= 1.0.5. An unknown, malformed or pre-release version reads as unsupported, so the existing hard error remains the fallback.
  • configure() and the --skip-upload path of search() read the server version before validate_dataset_support; on >= 1.0.5 labels is declared TAG SEPARATOR ;, the same declaration redis.rs uses and matching the ;-joined value upload already stores. create_index refuses to declare it without support even if validation is skipped.
  • The labels integration test now asserts filtered recall (>= 0.9) on the multi-valued fixture instead of rejection; the rejection path is covered by unit tests on both sides of the gate.
  • tests/docker-compose.test.yml: quay.io/kividbio/kividb:v1.0.2-full ->
    v1.0.5-full.
  • README / module docs: the kividb: metadata filtering broken against real KiviDB (TAG escaping + atomic tags); FT.CREATE SEPARATOR fixed #205 divergences re-measured on 1.0.5. Items 1-7 and 9 no longer reproduce; 8 (no GEO type) still does. The emitter is unchanged because its output is correct on every version.

Verification:

  • make agent-check: exit 0 (181 lib + 906 bin + invariant suites, fmt, clippy -D warnings).
  • integration_kividb against KiviDB 1.0.5: 21/21 passed; multi-valued labels recall 1.000.
  • Mutations, each restored after: dropping SEPARATOR from the labels declaration -> labels test recall 0.000, FAILED; gate min version 1.0.4 or always-true -> kividb_caps unit tests FAILED; rejecting labels regardless of version -> engine unit test FAILED.

….0.5-full

KiviDB 1.0.5 accepts FT.CREATE `TAG SEPARATOR` and splits stored values on
it, so the multi-valued `labels` field no longer needs to be a hard error.

- New `kividb_caps` library module (unit-tested): parses `kividb_version`
  from `INFO server` and gates multi-valued TAG support at >= 1.0.5. An
  unknown, malformed or pre-release version reads as unsupported, so the
  existing hard error remains the fallback.
- `configure()` and the `--skip-upload` path of `search()` read the
  server version before `validate_dataset_support`; on >= 1.0.5 `labels`
  is declared `TAG SEPARATOR ;`, the same declaration redis.rs uses and
  matching the `;`-joined value `upload` already stores. `create_index`
  refuses to declare it without support even if validation is skipped.
- The labels integration test now asserts filtered recall (>= 0.9) on the
  multi-valued fixture instead of rejection; the rejection path is covered
  by unit tests on both sides of the gate.
- tests/docker-compose.test.yml: quay.io/kividbio/kividb:v1.0.2-full ->
  v1.0.5-full.
- README / module docs: the redis#205 divergences re-measured on 1.0.5. Items
  1-7 and 9 no longer reproduce; 8 (no GEO type) still does. The emitter
  is unchanged because its output is correct on every version.

Verification:
- make agent-check: exit 0 (181 lib + 906 bin + invariant suites, fmt,
  clippy -D warnings).
- integration_kividb against KiviDB 1.0.5: 21/21 passed;
  multi-valued labels recall 1.000.
- Mutations, each restored after: dropping `SEPARATOR` from the labels
  declaration -> labels test recall 0.000, FAILED; gate min version
  1.0.4 or always-true -> kividb_caps unit tests FAILED; rejecting labels
  regardless of version -> engine unit test FAILED.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WdtvAfcNSzV85y5jAHMVr8
@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown

🤖 Automated first-pass review — a human maintainer's review is still required before merge.

What I checked: whether the version gate can fail open (taxonomy item 1, a filter that runs but silently matches nothing), whether the configure()-before-upload abort ordering still holds, the --skip-upload path, the timed-window placement of the new INFO round trip, per-config namespacing (unchanged, since index/key derivation isn't touched), and whether the tests actually guard what they claim. I found no correctness or measurement-fidelity defects. The gate fails closed in every case I could construct, and the mutation table in the PR body covers the ones that matter: no SEPARATOR gives 0.000 and goes red, gate at 1.0.4 or always-true goes red, rejecting regardless of version goes red.

Things the PR already gets right, so nobody has to rediscover them:

  • parse_kividb_version returns None for absent, malformed, 4-part or -rc versions, and supports_multivalued_tags(None) == false. An unknown server therefore keeps the pre-PR hard error. compares_numerically_not_lexically pins the 1.0.10 vs 1.0.5 trap.
  • does_not_mistake_redis_version_for_kividb_version matters here. Plain Redis on the KiviDB port would otherwise read as 8.10.0 and open the gate.
  • create_index refuses the multi-valued arm when !multivalued_tags (kividb.rs, new "keyword" if is_multivalued_keyword_field(..) arm). That's a real backstop rather than a comment. The arm is also ordered before the generic "keyword" | "uuid" | "bool" arm, which it has to be.
  • The INFO call in search() sits before read_queries() and the gate-synchronized worker start, so it's outside every timed window: one round trip per SearchParams, which is negligible.
  • The abort-before-upload ordering is still covered end to end. validate_dataset_support is the same call the geo integration test exercises through configure(). Removing the labels test's DBSIZE == 0 / assert_no_results_written assertions therefore doesn't leave the ordering unguarded.
  • The labels integration test is a real live guard on the parser too. If kividb_version weren't parsed from the real 1.0.5 reply, the run would hard-error and the test would go red. So INFO_105 isn't the only evidence the fixture shape matches.

Worth fixing before merge: nothing I'd call blocking.

Optional / follow-up:

  1. The old-server hard error doesn't say what the server reported. The validate_dataset_support message says "the server did not report kividb_version >= 1.0.5", but whoever hits it can't tell whether the field was absent, unparseable (a v1.0.5, 1.0.5-full, or 1.0.6+build style string would all read as None), or genuinely older. Passing the parsed Option<KividbVersion> (or the raw value) into validate_dataset_support and printing it would turn "why is my 1.0.6 rejected?" into a one-line diagnosis. The failure is safe either way; this is ergonomics only.
  2. CI no longer runs against any pre-1.0.5 server. Pinning v1.0.5-full in tests/docker-compose.test.yml means the emitter's "correct on every version, including 1.0.2" claim (the kividb_filter doc comment and the README re-measurement section) and the live None/old-version rejection path are now asserted only by unit tests and the one-off re-measurement. That's a reasonable trade, but it should be stated. If 1.0.2 support is meant to stay, a second compose service on the old tag running the labels-rejection and filter tests would keep both claims CI-guarded. Otherwise, consider saying in the README that pre-1.0.5 is best-effort.
  3. "Up to 1.0.4" overstates what was measured. The create_index comment says "Up to 1.0.4, FT.CREATE rejected SEPARATOR…", but the PR body and MULTIVALUED_TAG_MIN_VERSION's doc only cite measurements on 1.0.2 and 1.0.5. The gate is conservative either way (if 1.0.3 or 1.0.4 already split, the only cost is lost coverage), but the comment should say "1.0.2 (measured) … 1.0.5 (measured)" rather than imply 1.0.3 and 1.0.4 were probed.
  4. configure() now connects before rejecting geo. Geo rejection doesn't depend on the version, but with the connection and INFO now first, an unreachable server reports a connection error instead of the geo rejection. That's minor. Splitting the check into a schema-only pass before connecting and the version-dependent labels check after would keep the old "instant abort, no connection" property for geo.
  5. Label values containing ; get split, because both upload and index use the ; separator. That's at parity with redis.rs, so not a blocker. Worth a line in the KiviDB note now that labels is a supported path, next to the existing |/@/(/) paragraph.

The README's per-dataset table and the "2 of 61 on >= 1.0.5, 3 on older" count match the code paths above. The kividb_caps module lives in src/ with unit tests, as AGENTS.md asks.

Net: a clean, well-scoped extension. It fails closed on every unknown, the mutation testing backs up its claims, and the 1.0.5 re-measurement is careful (expected counts computed from the fixture rather than taken from either server). The optional items are small and local. Whether to drop live CI coverage of pre-1.0.5 KiviDB is a maintainer's call, and so is accepting the PR.

@fcostaoliveira

Copy link
Copy Markdown
Collaborator

Thank you for this, @murtazayusuf. The version gate, kividb_caps and the 1.0.5 re-measurement are a solid piece of work.

Maintainer edits are off on the fork, so I've continued it in #375. Your commit ce46925 is carried over unchanged as the first commit there, and you're credited as co-author for the squash. On top of it, #375:

  • merges current master (one conflict, in src/lib.rs);
  • addresses the automated review's points. The old-server error now names the reported kividb_version, geo is rejected before connecting, a kividb-legacy (v1.0.2) CI run keeps the pre-1.0.5 branch live-tested, and the README is updated;
  • fixes one thing the re-measurement surfaced: from 1.0.5 a bare TAG splits on , (RediSearch's default), so scalar keyword fields now get SEPARATOR ; too. Details and measurements are in the kividb: exact TAG filter values on 1.1.2 (escaping + live fixture), follow-up to #381 #375 description.

This PR will be closed when #375 merges. Comments on the follow-ups are very welcome there.

fcostaoliveira added a commit that referenced this pull request Oct 29, 2026
… and the FT.INFO check

#375 review pass 2, optional item 1: `"keyword" | "uuid" | "bool"` was
written out in `create_index`, `validate_dataset_support` and
`tag_field_names`. A TAG type added to `create_index` but not to
`tag_field_names` would drop out of the FT.INFO declaration check, so a
stale bare index on that field would pass `--skip-upload` silently.
`is_tag_type` is now the one definition, and
`tag_field_names_are_exactly_the_tag_typed_fields` pins the set.

The Co-authored-by trailer below credits #326's author in the default
squash message (this repo squashes with COMMIT_MESSAGES, and ce46925
itself is authored as Claude).

Co-authored-by: murtazayusuf <65263321+murtazayusuf@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@murtazayusuf

Copy link
Copy Markdown
Author

Superseded by #381, which carries @fcostaoliveira's follow-ups from #375 and moves CI to KiviDB 1.1.2. Closing this one.

fcostaoliveira added a commit that referenced this pull request Oct 29, 2026
…381)

Multi-valued TAG labels on KiviDB (SEPARATOR ;), with the FT.INFO separator
read-back on reused indexes. TAG support is gated on KiviDB >= 1.1.2: older
servers, or ones whose version can't be read, refuse TAG datasets and name
the version reported. CI runs kividb/kividb:v1.1.2-full from Docker Hub.

Supersedes #326. Follow-up: #375 (TAG value escaping on 1.1.2).

Co-authored-by: fcostaoliveira <fcostaoliveira@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.

3 participants