Skip to content

Fix #452: [Model] ConsistencyOfDatabaseFrequencyTables#718

Merged
GiggleLiu merged 12 commits intomainfrom
issue-452
Mar 21, 2026
Merged

Fix #452: [Model] ConsistencyOfDatabaseFrequencyTables#718
GiggleLiu merged 12 commits intomainfrom
issue-452

Conversation

@GiggleLiu
Copy link
Copy Markdown
Contributor

Summary

Add the implementation plan for ConsistencyOfDatabaseFrequencyTables, then execute it in follow-up commits.

Fixes #452

@GiggleLiu
Copy link
Copy Markdown
Contributor Author

Implementation Summary

Changes

  • Added the ConsistencyOfDatabaseFrequencyTables model with serializable frequency-table / known-value helpers, validation, evaluation, complexity metadata, registry exports, and focused model tests.
  • Added pred create ConsistencyOfDatabaseFrequencyTables support, including compact parsers for attribute domains, published frequency tables, and known values, plus CLI coverage for direct creation and canonical examples.
  • Added the ConsistencyOfDatabaseFrequencyTables -> ILP<bool> reduction using one-hot assignment variables and McCormick auxiliary variables, with closed-loop reduction tests.
  • Added the paper entry for Consistency of Database Frequency Tables with the formal definition and worked example.

Deviations from Plan

  • None.

Open Questions

  • None.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 20, 2026

Codecov Report

❌ Patch coverage is 98.73188% with 7 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@2fec9f9). Learn more about missing BASE report.
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...s/misc/consistency_of_database_frequency_tables.rs 96.33% 7 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #718   +/-   ##
=======================================
  Coverage        ?   97.62%           
=======================================
  Files           ?      399           
  Lines           ?    49720           
  Branches        ?        0           
=======================================
  Hits            ?    48537           
  Misses          ?     1183           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@GiggleLiu
Copy link
Copy Markdown
Contributor Author

Agentic Review Report

Structural Check

Structural Review: rule ConsistencyOfDatabaseFrequencyTables -> ILP

Structural Completeness

# Check Status
11 Paper reduction-rule entry FAIL — the new reduction is implemented in src/rules/consistencyofdatabasefrequencytables_ilp.rs:113 and registered for example export in src/rules/mod.rs:129, but docs/paper/reductions.typ:3097 only adds the model problem-def; there is no reduction-rule("ConsistencyOfDatabaseFrequencyTables", "ILP") block for the new edge.

Build Status

  • make test: PASS
  • make clippy: PASS

Semantic Review

  • paper quality: ISSUE — the code, tests, and exported reduction graph now include ConsistencyOfDatabaseFrequencyTables -> ILP, but the paper does not state or justify that direct reduction, so the documentation is structurally incomplete.

Summary

  • 10/11 structural checks passed
  • Missing paper coverage for ConsistencyOfDatabaseFrequencyTables -> ILP; the reduction exists in code, but the paper only documents the model and omits the new reduction rule.

Quality Check

Quality Review

Design Principles

  • DRY: OK
  • KISS: OK
  • HC/LC: OK

HCI (if CLI/MCP changed)

Test Quality

Issues

Critical (Must Fix)

None.

Important (Should Fix)

Minor (Nice to Have)

Summary

  • Important — misleading CDFT error messages reference a nonexistent --n flag.
  • Important — reduction tests miss the multi-table / non-uniform-domain cases that exercise the new indexing logic.
  • Minor — CDFT usage text says --known-values is required even though help and runtime behavior treat it as optional.

Agentic Feature Tests

Agentic Feature Test Report

Feature: ConsistencyOfDatabaseFrequencyTables
Worktree: /Users/jinguomini/rcode/problem-reductions/.worktrees/review-pr-718
Profile: ephemeral downstream CLI user
Use case: discover the model from the CLI catalog, inspect it, create the canonical example, and solve it using docs/examples alone
Expected outcome: the full pred listpred showpred create --examplepred solve path works, and the explicit reduction to ILP also works
Verdict: pass with 1 confirmed setup/docs issue

Issues Found

  1. Medium: README says make cli “builds target/release/pred”, but it actually performs a global cargo install and replaces the user’s existing pred.
    • Status: confirmed in current worktree
    • Evidence: README.md:40 vs Makefile:170 and Makefile:171
    • Reproduction:
      1. cd /Users/jinguomini/rcode/problem-reductions/.worktrees/review-pr-718
      2. make cli
      3. Observe cargo install --path problemreductions-cli and Replacing /Users/jinguomini/.cargo/bin/pred
    • Impact: the documented “build from source” path writes outside the repo and can overwrite an already-installed pred, which is surprising for review-pipeline and for downstream users.

Summary

Check Result Notes
pred list Pass ConsistencyOfDatabaseFrequencyTables * appears in the catalog with Rules = 1
pred show ConsistencyOfDatabaseFrequencyTables Pass Shows description, 4 fields, 5 size fields, and 1 outgoing reduction to ILP/bool
pred create --example ConsistencyOfDatabaseFrequencyTables Pass Canonical example emitted successfully
pred solve cdf.json Pass Solved via ILP with evaluation true
pred reduce cdf.json --to ILP Pass Produced a 1-step bundle ConsistencyOfDatabaseFrequencyTables -> ILP
pred solve cdf-to-ilp.json Pass Bundle solved successfully

Per-Feature Details

  • Discoverability: good. Starting from the generic CLI docs and the catalog, the model was easy to find with pred list, and pred show exposed the reduction path clearly.
  • Setup: partial. The build succeeded, but the README’s make cli description is inaccurate.
  • Functionality: good. The canonical example was created and solved successfully, and the explicit reduction flow also worked.
  • Expected vs actual: matched. The downstream-user scenario completed end-to-end without needing source inspection.
  • Blocked steps: skipped the crates.io install path intentionally, because the goal was to test the PR worktree build, not the published crate.
  • Friction points: there is no model-specific CLI example in the general CLI docs, but the generic pred create --example <PROBLEM_SPEC> guidance plus the paper example were sufficient.

Commands Executed

  1. pred list
  2. pred show ConsistencyOfDatabaseFrequencyTables
  3. pred create --example ConsistencyOfDatabaseFrequencyTables -o cdf.json
  4. pred solve cdf.json
  5. pred reduce cdf.json --to ILP -o cdf-to-ilp.json
  6. pred solve cdf-to-ilp.json

I did not save a report file under docs/test-reports/ because this run was requested to stay read-only.


Generated by review-pipeline

@GiggleLiu
Copy link
Copy Markdown
Contributor Author

Follow-up items (recorded during final review):

  • ensure_attribute_indices_in_range() in create.rs:194 hardcodes --n in error message — should parameterize the flag name for CDFT context
  • CDFT usage string in create.rs:1812 shows --known-values as required, but it's optional

@GiggleLiu GiggleLiu merged commit 7952b3d into main Mar 21, 2026
3 checks passed
@GiggleLiu GiggleLiu deleted the issue-452 branch April 12, 2026 00:53
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.

[Model] ConsistencyOfDatabaseFrequencyTables

1 participant