DEV-1480: structured Column.sampled_values + distinct_count#149
Merged
ZmeiGorynych merged 3 commits intoMay 27, 2026
Conversation
Adds two new optional Column fields alongside the existing sampled text: sampled_values (top-50-by-frequency list) and distinct_count (true cardinality at profile time). Lets consumers compare predicate literals against actual stored values without ambiguity from comma-containing values (e.g. "R$ 1,000-3,000"). SlayerModel version bumps 6 -> 7 with a no-op forward migration. Categorical profiling rewritten to order by per-value count desc with SQL-side alphabetical tie-break and absorb a NULL row via LIMIT + 2. On overflow > 50 distinct, fires a secondary count_distinct query via ModelExtension so the persisted total is exact, not capped, and the column's allowed_aggregations whitelist + filter don't interfere. Text sampled format gains a "top20 ... (N distinct)" suffix on overflow. inspect_model lives on the new ColumnSample shape, uses _is_sample_cached for cache validity (categorical needs sampled_values is not None so v6 legacy rows re-profile), removes the max_dims=10 cap on the persistence path, and uses key-presence not truthiness in row construction so empty strings don't fall through. _collect_measure_profile is restricted to numeric/temporal types. JSON output gains sampled_values + distinct_count; markdown table is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
More reviews will be available in 37 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR implements DEV-1480 structured categorical sampling: it extends the ChangesStructured Categorical Sampling (DEV-1480)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
Member
Author
|
@coderabbitai review this now! |
Contributor
|
(ﾉ◕ヮ◕)ﾉ*:･ﾟ✧ ✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/concepts/models.md (1)
41-42:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale
SlayerModelversion references to7.This page still says current
SlayerModelversion is6, but this PR introduces v7. Please update both mentions to avoid conflicting migration guidance.Also applies to: 420-420
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/concepts/models.md` around lines 41 - 42, Update the stale SlayerModel schema version from 6 to 7 wherever the `version` field is documented (e.g., the table row showing `version` and any textual references to "SlayerModel version 6"); change the int value and any explanatory text to `7` so the docs reflect the new v7 schema and avoid conflicting migration guidance.
🧹 Nitpick comments (3)
tests/test_storage_type_refinement.py (1)
286-287: ⚡ Quick winMove
migrationsimport to module scope.
migis imported inside test bodies twice; per repo convention, imports should be at the top of the file.♻️ Suggested cleanup
import pytest import sqlalchemy as sa import yaml from slayer.core.enums import DataType from slayer.core.models import DatasourceConfig +from slayer.storage import migrations as mig from slayer.storage.yaml_storage import YAMLStorage @@ - from slayer.storage import migrations as mig - await storage_with_v4_model["storage"].get_model("items", data_source="live") @@ - from slayer.storage import migrations as mig assert raw["version"] == mig.CURRENT_VERSIONS["SlayerModel"]As per coding guidelines:
**/*.py: Place imports at the top of files.Also applies to: 442-443
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_storage_type_refinement.py` around lines 286 - 287, The tests import "mig" (from slayer.storage import migrations as mig) inside test functions twice; move that import to module scope by adding "from slayer.storage import migrations as mig" at the top of tests/test_storage_type_refinement.py, remove the duplicate in-function imports, and run tests to ensure no shadowing or name conflicts with other top-level imports.tests/integration/test_mcp_inspect.py (2)
649-671: ⚡ Quick winAlso pin that re-profiling refreshes
sampled.This test only proves the new structured fields are repopulated. If the legacy text sample survives, JSON/markdown can still serve stale profile text and this would still pass.
Suggested assertion
server = create_mcp_server(storage=storage) - await self._call_json(server, model_name="orders") + payload = await self._call_json(server, model_name="orders") + status_payload = next(c for c in payload["columns"] if c["name"] == "status") + assert status_payload["sampled"] != "legacy text from v6" # After inspect_model runs, the structured field has been populated. reloaded = await storage.get_model("orders", data_source="test_sqlite") status = reloaded.get_column("status") + assert status.sampled != "legacy text from v6" assert status.sampled_values is not None assert status.distinct_count == 3🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/test_mcp_inspect.py` around lines 649 - 671, Add an assertion to ensure the legacy text field `sampled` is refreshed when re-profiling: after reloading the model in test_v6_stale_cache_re_profiles_to_populate_sampled_values (and after calling storage.update_column_sampled/inspect_model), assert that the column's `sampled` attribute is not the original legacy string (e.g., not "legacy text from v6") and/or is cleared/updated alongside `sampled_values`, so the old freeform sample text cannot leak through.
718-763: ⚡ Quick winAssert
distinct_counton the wide-model path too.Right now this only proves
sampled_valuespersists past the old cap. If columns 11+ stop writingdistinct_count, DEV-1480 regresses and the test still passes.Suggested assertion
for i in range(col_count): col = reloaded.get_column(f"c{i}") assert col.sampled_values is not None, ( f"c{i}.sampled_values is None — max_dims cap not removed" ) + assert col.distinct_count == 1, ( + f"c{i}.distinct_count was not persisted" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/test_mcp_inspect.py` around lines 718 - 763, The test test_profiles_more_than_10_categorical_columns currently only asserts col.sampled_values persists; add an assertion that the persisted Column.distinct_count is also present for every categorical column after reloading. Locate the loop that calls reloaded.get_column(f"c{i}") and add a check such as assert col.distinct_count is not None (or > 0 if appropriate) alongside the existing sampled_values assertion to ensure columns 11+ still write distinct_count.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/concepts/models.md`:
- Around line 41-42: Update the stale SlayerModel schema version from 6 to 7
wherever the `version` field is documented (e.g., the table row showing
`version` and any textual references to "SlayerModel version 6"); change the int
value and any explanatory text to `7` so the docs reflect the new v7 schema and
avoid conflicting migration guidance.
---
Nitpick comments:
In `@tests/integration/test_mcp_inspect.py`:
- Around line 649-671: Add an assertion to ensure the legacy text field
`sampled` is refreshed when re-profiling: after reloading the model in
test_v6_stale_cache_re_profiles_to_populate_sampled_values (and after calling
storage.update_column_sampled/inspect_model), assert that the column's `sampled`
attribute is not the original legacy string (e.g., not "legacy text from v6")
and/or is cleared/updated alongside `sampled_values`, so the old freeform sample
text cannot leak through.
- Around line 718-763: The test test_profiles_more_than_10_categorical_columns
currently only asserts col.sampled_values persists; add an assertion that the
persisted Column.distinct_count is also present for every categorical column
after reloading. Locate the loop that calls reloaded.get_column(f"c{i}") and add
a check such as assert col.distinct_count is not None (or > 0 if appropriate)
alongside the existing sampled_values assertion to ensure columns 11+ still
write distinct_count.
In `@tests/test_storage_type_refinement.py`:
- Around line 286-287: The tests import "mig" (from slayer.storage import
migrations as mig) inside test functions twice; move that import to module scope
by adding "from slayer.storage import migrations as mig" at the top of
tests/test_storage_type_refinement.py, remove the duplicate in-function imports,
and run tests to ensure no shadowing or name conflicts with other top-level
imports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 347542fb-c101-46bc-8229-a02154026d36
📒 Files selected for processing (23)
CLAUDE.mddocs/concepts/models.mddocs/concepts/search.mdslayer/core/models.pyslayer/engine/profiling.pyslayer/mcp/server.pyslayer/search/render.pyslayer/storage/base.pyslayer/storage/join_sync.pyslayer/storage/migrations.pyslayer/storage/sqlite_storage.pyslayer/storage/v7_migration.pyslayer/storage/yaml_storage.pytests/integration/test_mcp_inspect.pytests/test_engine_profiling.pytests/test_migrations.pytests/test_models.pytests/test_search_render.pytests/test_storage_sampled.pytests/test_storage_type_refinement.pytests/test_v4_migration.pytests/test_v6_migration.pytests/test_v7_migration.py
Group A (slayer/engine/profiling.py): - Codex: _profile_categorical_with_total now drops sampled_values to None when the secondary count_distinct query fails so the column is classified as cache-miss and retried next call (instead of being permanently stuck with distinct_count=None). - Sonar S3776: extract _refresh_one_column helper to reduce cognitive complexity in refresh_table_backed_model_sampled below the 15-cap. Group B (slayer/mcp/server.py): - Codex: split inspect_model live-miss loop into categorical (per-column via profile_column) and numeric/temporal (one batched min/max via _profile_numeric_temporal_columns). Restores pre-DEV-1480 batching for wide models so N numeric columns no longer cost N round trips. Group C (tests): - Sonar S1481: rename unused 'i' to '_' in test_engine_profiling.py. - Sonar S7503: NOSONAR on test_mcp_inspect.py injected_measure_profile (must be async to monkeypatch the async _collect_measure_profile). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
- slayer/mcp/server.py: in inspect_model, seed profile_by_name with the legacy ``sampled`` text when a column is classified uncached. v6-upgraded categorical columns (sampled set, sampled_values=None) now retain a visible cell if the live re-profile fails for transient backend reasons. Codex finding. - tests/test_storage_type_refinement.py: hoist ``from slayer.storage import migrations as mig`` from two inline test bodies to the module-level imports per ``feedback_no_inline_imports.md``. CodeRabbit nitpick. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
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
Column.sampled_values: Optional[List[str]](top-50-by-frequency list) andColumn.distinct_count: Optional[int](true cardinality) alongside the existingColumn.sampledtext. BumpsSlayerModel.version6 → 7 (no-op forward migration).ORDER BY _count DESC, value ASC,LIMIT max_values + 2to absorb a NULL row, and a secondarycount_distinctviaModelExtensionon overflow > 50 — bypassesColumn.allowed_aggregationswhitelist +Column.filterso the persisted total is the column's raw cardinality.inspect_modelJSON gainssampled_values+distinct_count; markdown table unchanged. Cache validity rule_is_sample_cachedmakes v6 legacy rows re-profile on next call._collect_measure_profilerestricted to numeric/temporal types; row construction switched to key-presence (empty-stringsampledno longer falls through to the measure-profile fallback).Motivation
Resolves DEV-1480. The text
sampledis ambiguous for values containing commas (e.g."R$ 1,000–3,000") — downstream consumers (DEV-1478's literal-existence validator) need an unambiguous structured list to compare predicate literals against. The structured field also lets validators surface the true cardinality and the most-common-first ordering for high-cardinality columns.Test plan
poetry run pytest -m "not integration"— 3200 passed / 4 skipped / 0 failed.poetry run ruff check slayer/ tests/.tests/test_v7_migration.pypins the v6→v7 no-op forward and the new field defaults.tests/test_engine_profiling.pyextended with frequency ordering, NULL absorption at LIMIT boundary, overflow with allowed_aggregations whitelist, overflow ignoring Column.filter, tie-break determinism, comma-containing values, ColumnSample return shape, refresh path passing all three kwargs.tests/test_storage_sampled.pyround-trips all three fields through YAML / SQLite / JoinSync.tests/test_search_render.pypins content_hash stability for the new fields and empty-string skip.tests/integration/test_mcp_inspect.pyextended with JSON-output coverage, v6 stale-cache re-profile, all-NULL → empty string (not"all NULL"), > 10 categorical columns all profiled, measure_profile type restriction, and a targeted truthiness regression with a monkeypatched fallback.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation