V0.5.0/code review fixes#162
Merged
Merged
Conversation
…e facts
M0.5 — Feasibility Spikes
- Spike A: callback as {:callback, %IR.Callback{}} IR literal round-trips
through compiler/evaluator (chest/spikes/README.md)
- Spike B: materialize(goal:) drives magic sets transform; results ≡ semi-naive
M1 — Query Planner Foundation - closed #127
- Add ExDatalog.Planner with plan/2, explain_plan/1,2 - closed #128
- Add planner structs: Plan, Stratum, Join, Predicate (lib/ex_datalog/planner/) - closed #129
- Engine strategy dispatch: evaluate/2 routes :semi_naive / :magic_sets - closed #130
- Telemetry [:ex_datalog, :planner, :start|:stop|:exception] - closed #131
- Tests: planner_test.exs, planner_explain_test.exs - closed #132
M2 — Aggregates (count/sum/min/max) - closed #133
- Add @aggregate_ops [:count, :sum, :min, :max] to Constraint;
from_tuple/1, valid_right?/2, valid_result?/2 extended - closed #134
- Add ExDatalog.Constraints.Aggregate (group_and_reduce/5; evaluate/3 raises) - closed #135
- Add Rule.has_aggregates?/1, aggregate_constraints/1, non_aggregate_constraints/1 - closed #136
- Safety: aggregate input var must be bound, result var in head, ≤1 agg/rule,
no aggregate in recursive rule - closed #137
- Stratification: post-pass in assign_strata/1 forces aggregate head above
all positive body relations; engine validate_stratification/1 extended - closed #138
- Evaluator: eval_aggregate_rule/3 (join → filter → callbacks → negation →
group_and_reduce → project); apply_constraints/3 partitions out aggregate ops - closed #139
- DSL: count/sum/min/max(X, Result) parsed before generic constraint-ops loop;
head-position aggregate raises DSL.CompileError - closed #140
- Tests: aggregate_test.exs + DSL tests in schema_test.exs - closed #141
M3 — BEAM Callback Predicates - closed #142
- Add ExDatalog.Callback struct (AST) + IR.Callback struct + {:callback, _} IR literal - closed #143
- Add ExDatalog.Constraints.BeamCallback with apply_callback/3;
spawn_monitor-based isolation (not Task.async) - closed #144
- Add Constraint.Context opts: [] field; threaded from Engine.Naive.do_evaluate_inner - closed #145
- Add Capabilities aggregate_constraints: boolean, beam_callbacks: boolean - closed #146
- Add Schema.PredicateMeta struct; predicate/5 macro; __register_predicate__/6 - closed #147
- Add resolve_positive_or_callback/2 in __build_program__;
converts matching body atoms to callbacks - closed #148
- Add Validator.check_callback/2 (module/function/arity),
Safety.check_callback_inputs/2 (args must be bound) - closed #149
- Evaluator: apply_callbacks/3 in normal and aggregate paths;
apply_callback_chain/2 + step_callback/1 - closed #150
- Program.validate_body/1 accepts {:callback, %Callback{}};
body_relations/1 ignores callbacks - closed #151
- Tests: beam_callback_test.exs - closed #152
M4 — Magic Sets (Experimental) - closed #153
- Add ExDatalog.MagicSets: transform/2 → {:ok, ir} | {:fallback, reason} - closed #154
- Scope: positive recursive programs, single goal, ground bound positions;
negation/aggregates → fallback - closed #155
- Transform: adornment → magic relation + seed fact → rewrite goal-relation
rules (prepend magic atom for bound head positions) - closed #156
- bound_positions/1 maps goal bound positions to head term positions - closed #157
- Verified: magic restricts derivation and equals semi-naive subset - closed #158
- Tests: magic_sets_test.exs + magic_sets_property_test.exs - closed #159
M5 — Docs, Benchmarks, CHANGELOG, Version Bump - closed #160
- Version bumped 0.4.1 → 0.5.0 in mix.exs
- Added {:benchee, "~> 1.3", only: :dev} dependency
- Added planner/magic-sets/callback module groups to mix.exs docs
- Added articles 06–09 and migration_v0.5.md as extras
- CHANGELOG.md: full v0.5.0 entry (Added/Changed/Notes)
- README.md: aggregate/callback/magic-sets sections, updated feature list,
architecture diagram, constraints table, docs links, roadmap
- Articles: 06_query_planning_in_datalog.md, 07_aggregates_in_datalog.md,
08_extending_datalog_with_beam_callbacks.md,
09_magic_sets_and_demand_driven_evaluation.md
- Migration guide: docs/migration_v0.5.md
- Livebook: livebooks/ex_datalog_v050.livemd (DSL edition)
- Benchmarks: bench/aggregate_bench.exs, bench/magic_sets_bench.exs
- Report: chest/benchmarks/v0.5.0-report.md
Runtime Facts API (pipe-friendly Schema → Program flow)
- Schema relation constructors: each relation generates a public function
(e.g., DeptCount.emp(:alice, :eng) → {"emp", [:alice, :eng]})
- Schema.new/0: returns blank program with relations + rules but no facts
- Program.add_fact/2 (tuple form): Program.add_fact(prog, {"emp", [:alice, :eng]})
- Program.add_facts/2: bulk addition from list of {relation, values} tuples
- Program.materialize/1,2: pipe-friendly ExDatalog.materialize/2 wrapper
- Schema.program/0 and materialize/1 unchanged for backward compatibility
…s not release-ready yet. The branch has broad test coverage and the normal verification commands pass, but the review found several material issues where public API behavior, documentation, and tests diverge from the intended v0.5.0 feature set.
The most important issue is that the public `ExDatalog.materialize/2` API strips `:goal` before evaluation, so `strategy: :magic_sets` does not receive the goal it needs to run the magic-sets transformation. Current magic-sets tests still pass because `materialize/2` post-filters results after full semi-naive evaluation, which masks the issue.
Other release-relevant issues include aggregate validation gaps, non-integer aggregate crashes, pipe-friendly runtime-facts error handling, callback timeout mailbox leakage, invalid README examples, and stale Schema documentation.
Release readiness: **Needs Significant Fixes**.
| Rank | Severity | Category | Location | Issue | Fix Effort |
|---|---|---|---|---|---|
| 1 | High | Correctness / Public API | `lib/ex_datalog.ex:217-223`, `lib/ex_datalog/engine/naive.ex:102-115` | Public `materialize(strategy: :magic_sets, goal: ...)` strips `:goal`, so magic sets does not run through the advertised API | M |
| 2 | High | Correctness | `lib/ex_datalog/validator/safety.ex:268-274`, `lib/ex_datalog/engine/evaluator.ex:134-141` | Aggregate result variable is not required to appear in the head, so aggregate work can be silently discarded | S |
| 3 | High | Correctness / Error Handling | `lib/ex_datalog/constraints/aggregate.ex:58-61` | Integer-only aggregate contract is not enforced; `sum` can crash and `min`/`max` accept non-integers | M |
| 4 | High | Public API / Error Handling | `lib/ex_datalog/program.ex:295-298` | `Program.materialize/1` crashes on `{:error, reason}` in pipe chains instead of propagating errors | XS |
| 5 | Medium | Concurrency / Resource Leak | `lib/ex_datalog/constraints/beam_callback.ex:89-101` | Timed-out callbacks can leave late result messages in the evaluator mailbox | S |
| 6 | Medium | Public API / Planner | `lib/ex_datalog/planner.ex:166-171`, `livebooks/ex_datalog_v050.livemd:718-753` | Planner docs claim callbacks are classified, but planner ignores callback literals | S |
| 7 | Medium | Documentation | `README.md:240-291` | New README aggregate/callback examples use unsupported DSL syntax and invalid materialization calls | S |
| 8 | Medium | Documentation | `lib/ex_datalog/schema.ex:131-140`, `lib/ex_datalog/schema.ex:248-249`, `lib/ex_datalog/schema.ex:997` | Schema docs are stale: aggregates described as unsupported, `new_program/0` referenced, callback validation timing wrong | S |
| 9 | Medium | Tests | `test/ex_datalog/magic_sets_test.exs:35-47`, `test/ex_datalog/magic_sets_property_test.exs:41-57` | Magic-sets tests verify result equality but not that the public API used the transform | M |
| 10 | Low | Documentation / Examples | `livebooks/ex_datalog_v050.livemd`, `livebooks/ex_datalog_v050-v2.livemd` | v0.5 livebook still uses compile-time facts for runtime-data scenarios; duplicate `-v2` fork contains invalid API | S |
| 11 | Low | Generated Docs / Edge Case | `lib/ex_datalog/schema.ex:218-239` | Generated relation constructor docs are malformed; zero-arity relations generate warnings / wrong arguments | S |
| 12 | Low | Tests | `test/ex_datalog/runtime_facts_test.exs:153-162` | `Program.add_facts/2` error test does not assert partial-add semantics or malformed input behavior | XS |
| 13 | Nit | LLM Artifact / Style | README, livebook, articles | Marketing phrasing, long dashes, repetitive generated-style prose, and fork artifact remain | S |
No Critical severity findings were identified. The branch does have High severity issues that should block release, but the reviewed bugs do not appear to corrupt persisted data or produce unsound query answers in the default semi-naive path.
```text
ID: H-001
Severity: High
Category: Correctness / Public API
Location: lib/ex_datalog.ex:217-223; lib/ex_datalog/engine/naive.ex:102-115; test/ex_datalog/magic_sets_test.exs:35-47
Summary: The public materialize(strategy: :magic_sets, goal: ...) API strips :goal before evaluation, so the magic-sets transform does not run through the advertised API.
Evidence: ExDatalog.materialize/2 does `{goal, eval_opts} = Keyword.pop(opts, :goal, nil)` and then calls `evaluate(ir, eval_opts)`. Engine.Naive.evaluate_magic_sets/2 requires `Keyword.get(opts, :goal, nil)` to call MagicSets.transform/2; without it, it falls back to evaluate_semi_naive/2. The tests at magic_sets_test.exs:42-46 still pass because materialize/2 later applies apply_goal/2, so results look goal-restricted after full evaluation.
Impact: The advertised v0.5.0 magic-sets feature is not active through the main public API. Users get full semi-naive evaluation plus post-filtering, so benchmarks, docs, and performance claims are misleading. This also invalidates tests that claim to prove goal-driven materialization.
Recommended Fix: Preserve the goal for evaluation when `strategy: :magic_sets` is selected. Separate the concepts of evaluator goal (for magic sets) and post-materialization filtering, e.g. keep `:goal` in `eval_opts` for `strategy: :magic_sets` and still apply post-filtering afterward if that is the desired public result shape. Consider renaming the post-filter option in a future release to avoid semantic overload.
Suggested Test: Add a regression test proving public `ExDatalog.materialize(program, strategy: :magic_sets, goal: goal)` invokes the magic-sets path, not just post-filtering. One practical approach is to add telemetry/metadata from the engine indicating `strategy: :magic_sets` and `magic_transform: :applied | :fallback`, then assert `:applied` for a supported goal. Alternatively expose enough stats to distinguish transformed evaluation from post-filtered full evaluation.
Confidence: High
```
```text
ID: H-002
Severity: High
Category: Correctness / Validation
Location: lib/ex_datalog/validator/safety.ex:268-274; lib/ex_datalog/engine/evaluator.ex:134-141
Summary: Aggregate result variables are not required to appear in the rule head, so aggregate computations can be silently discarded.
Evidence: check_aggregates/3 only calls check_single_aggregate/4 and check_aggregate_not_recursive/4. There is no check that `Constraint.result_variable(aggregate)` appears in `Rule.head_variables(rule)`. Reproduction: a rule `dept(D) :- emp(E, D), count(E, N)` validates and materializes as `dept(:eng)` while silently computing and then discarding `N` during projection.
Impact: This violates the planned aggregate safety rule and can hide user mistakes. A user can believe they are computing an aggregate while the output relation contains only group keys. This is especially risky because materialization succeeds without an error.
Recommended Fix: Add a safety check that each aggregate result variable appears in the rule head. Return a specific validator error such as `:aggregate_result_not_in_head` with rule index, op, and variable.
Suggested Test: Add builder and DSL tests where `count(E, N)` appears in the body but `N` is absent from the head. Assert validation/materialization returns an error and the DSL raises/returns `DSL.CompileError` where appropriate.
Confidence: High
```
```text
ID: H-003
Severity: High
Category: Correctness / Error Handling
Location: lib/ex_datalog/constraints/aggregate.ex:58-61; docs/articles/07_aggregates_in_datalog.md; lib/ex_datalog/constraints/aggregate.ex:12-14
Summary: The integer-only aggregate contract is not enforced at runtime.
Evidence: Aggregate.compute/2 uses `Enum.sum/1`, `Enum.min/1`, and `Enum.max/1` directly. A `sum` aggregate over a `:any` relation containing `:a` validates but raises `ArithmeticError: bad argument in arithmetic expression` during materialization. `min`/`max` over atom values succeed using Erlang term ordering, despite docs saying aggregates are integer-only.
Impact: Invalid user data can crash evaluation instead of returning a structured error or filtering/rejecting the program. The behavior contradicts documentation and the v0.5.0 scope statement.
Recommended Fix: Validate aggregate input relation field types where statically knowable, and guard runtime values in group_and_reduce/5. For `sum`, reject non-integers with a structured evaluation error. For `min`/`max`, either explicitly allow all comparable Erlang terms and update docs, or enforce integers consistently.
Suggested Test: Add tests for `sum`, `min`, and `max` over non-integer facts. Assert the chosen behavior: validation error if statically detectable, or `{:error, reason}` from materialization rather than an exception.
Confidence: High
```
```text
ID: H-004
Severity: High
Category: Public API / Error Handling
Location: lib/ex_datalog/program.ex:295-298
Summary: Program.materialize/1 is not error-pipe-safe.
Evidence: Program.add_fact/2 can return `{:error, reason}`. Program.materialize/1 only matches `%Program{}`. Reproduction: `Program.new() |> Program.add_relation("emp", [:atom, :atom]) |> Program.add_fact({"missing", [:x]}) |> Program.materialize()` raises `FunctionClauseError`, whereas `ExDatalog.materialize({:error, _})` propagates the error.
Impact: The newly introduced pipe-friendly runtime facts API fails in exactly the case where pipe-friendly APIs are expected to preserve error tuples. Users get an exception instead of an actionable `{:error, reason}`.
Recommended Fix: Add `def materialize({:error, _} = error, _opts \\ []), do: error` or equivalent non-conflicting clauses. Also consider adding `Program.add_facts/2` malformed input handling.
Suggested Test: Add a test for `Program.new() |> Program.add_relation(...) |> Program.add_fact({"missing", [...]}) |> Program.materialize()` returning `{:error, _}`.
Confidence: High
```
```text
ID: M-001
Severity: Medium
Category: Concurrency / Resource Leak
Location: lib/ex_datalog/constraints/beam_callback.ex:89-101
Summary: Timed-out callbacks can leave late result messages in the evaluator process mailbox.
Evidence: On timeout, safe_apply/4 runs `Process.exit(pid, :kill)`, demonitor-flushes the monitor, and returns `{:error, :timeout}`. If the child sends `{ref, result}` just after the timeout branch is selected but before it is killed, that message is not flushed. Since each callback uses a unique ref, later receive blocks will skip the stale message, leaving it in the mailbox.
Impact: Workloads with many timed-out callbacks can accumulate unmatched messages in the evaluator process mailbox, increasing memory use and receive scanning cost.
Recommended Fix: After killing/demonitoring, opportunistically flush `{^ref, _}` with a zero-timeout receive. Alternatively use a single message protocol where the child exits with the result and the parent only consumes the monitor message, or ensure both result and DOWN are drained for the ref.
Suggested Test: Add a controlled callback that sleeps close to the timeout and sends late results repeatedly; assert the caller mailbox does not grow with stale `{ref, result}` messages. This may require exposing a test-only helper or using process_info(self(), :messages) in a focused unit test.
Confidence: Medium
```
```text
ID: M-002
Severity: Medium
Category: Public API / Planner
Location: lib/ex_datalog/planner.ex:166-171; lib/ex_datalog/planner.ex:2-8; livebooks/ex_datalog_v050.livemd:718-753
Summary: Planner documentation says callbacks are predicates, but build_predicates/1 ignores callback literals.
Evidence: Planner moduledoc says the plan includes predicates "(constraints, aggregates, callbacks)". build_predicates/1 only collects `for {:constraint, c} <- rule.body`. The livebook section says planner classifies `:callback`, but the shown callback plan output has `Predicates: 0`.
Impact: Planner output is incomplete for callback-heavy rules, and docs/examples contradict implementation. Future users relying on Planner for explainability or optimization will miss callback predicates.
Recommended Fix: Extend build_predicates/1 to include `{:callback, cb}` as `%Predicate{kind: :callback, op: :callback, metadata: ...}` or update docs to state callbacks are not included in v0.5.0 plans. The former better matches the public claim.
Suggested Test: Add a planner test with a DSL `predicate/5` callback in a rule body and assert `plan.predicates` contains one `kind: :callback` predicate and `explain_plan/2` includes `callback`.
Confidence: High
```
```text
ID: M-003
Severity: Medium
Category: Documentation / Examples
Location: README.md:240-291
Summary: New README aggregate and callback examples use unsupported Schema DSL syntax.
Evidence: README uses `relation "emp", [:atom, :atom]`, `fact "emp", [:alice, :eng]`, and `ExDatalog.materialize(DeptStats)`. The implemented Schema DSL uses `relation :emp do field ... end`, `fact(emp(...))`, and module-generated `DeptStats.materialize()` / `DeptStats.program()`.
Impact: Users copying the v0.5.0 README examples will get compile errors and may conclude the new aggregate/callback APIs are broken.
Recommended Fix: Rewrite README v0.5.0 feature examples using the actual DSL. Given the runtime facts discussion, prefer `Schema.new() |> Program.add_fact(Schema.rel(...)) |> Program.materialize()` for realistic data-loading examples.
Suggested Test: Add README example snippets to doctests or a docs/examples test harness that compiles and runs them.
Confidence: High
```
```text
ID: M-004
Severity: Medium
Category: Documentation / API Docs
Location: lib/ex_datalog/schema.ex:131-140; lib/ex_datalog/schema.ex:248-249; lib/ex_datalog/schema.ex:997
Summary: Schema documentation is stale and contradicts v0.5.0 implementation.
Evidence: Schema moduledoc still says "Aggregates are not yet supported" and planned for v0.6.0. Generated `program/0` docs say "For runtime facts, use new_program/0" even though the agreed API is `new/0`. predicate/5 docs say missing callback module/function raises a compile-time DSL.CompileError; reproduction shows the schema module compiles and `materialize()` returns `{:error, [%Validator.Error{kind: :invalid_callback}]}`.
Impact: API docs mislead users on core v0.5.0 features and error timing. This is especially risky because `Schema` is the main DSL entry point.
Recommended Fix: Update Schema moduledoc to describe supported `count/sum/min/max`, runtime facts via relation constructors + `new/0`, and actual callback validation timing. Remove or reframe the old `agg(...)` preview as a legacy placeholder error.
Suggested Test: Run `mix docs --warnings-as-errors` already passes, but add docs-focused tests that compile examples in Schema docs where practical.
Confidence: High
```
```text
ID: M-005
Severity: Medium
Category: Testing
Location: test/ex_datalog/magic_sets_test.exs:35-47; test/ex_datalog/magic_sets_property_test.exs:41-57
Summary: Magic-sets tests would pass even if magic sets never ran through the public API.
Evidence: The tests compare `Knowledge.match(magic, "ancestor", [:a, :_])` to a subset of full semi-naive results. Because ExDatalog.materialize/2 post-filters by `:goal`, this assertion passes even when Engine.Naive receives no goal and falls back to full semi-naive evaluation.
Impact: The highest-value magic-sets regression is not protected. H-001 reached the branch despite "magic≡semi-naive" property tests.
Recommended Fix: Add tests that distinguish transformation from post-filtering. Instrument the engine/magic transform path with telemetry or stats, then assert transform application/fallback reason. Also test that `goal` reaches the engine when `strategy: :magic_sets` is requested.
Suggested Test: Attach a telemetry handler to a new event such as `[:ex_datalog, :magic_sets, :stop]` and assert metadata `%{status: :applied}` for a supported program and `%{status: :fallback, reason: ...}` for unsupported programs.
Confidence: High
```
```text
ID: M-006
Severity: Medium
Category: Documentation / Examples
Location: livebooks/ex_datalog_v050.livemd; livebooks/ex_datalog_v050-v2.livemd
Summary: The v0.5.0 livebook does not reflect the agreed runtime-facts DSL pattern, and a duplicate fork contains invalid API calls.
Evidence: livebooks/ex_datalog_v050.livemd still uses compile-time `fact(...)` macros for employee/user datasets. livebooks/ex_datalog_v050-v2.livemd contains calls like `DeptCount.fact(DeptCount.emp(:alice, :eng))`, but no such `DeptCount.fact/1` function exists.
Impact: The primary v0.5.0 tutorial does not teach the realistic data-loading pattern that motivated `Schema.new/0`, relation constructors, `Program.add_fact/2`, and `Program.add_facts/2`. Accidentally committing the `-v2` fork would ship broken examples.
Recommended Fix: Rewrite the livebook examples to use `DeptCount.new() |> Program.add_facts([...]) |> Program.materialize()`. Delete or ignore `livebooks/ex_datalog_v050-v2.livemd` unless it is intentionally kept as a draft outside git.
Suggested Test: Add a lightweight script/test that extracts and evaluates Livebook Elixir cells, or maintain a mirrored `.exs` example file for CI.
Confidence: High
```
```text
ID: L-001
Severity: Low
Category: Generated Documentation / Edge Case
Location: lib/ex_datalog/schema.ex:218-239
Summary: Generated relation constructor documentation is malformed, and zero-arity relations generate warnings / wrong argument ranges.
Evidence: The generated doc interpolates `#=> {"rel", [arg_1, arg_2})]}` with mismatched parentheses/brackets. For an empty relation block, the generator uses `1..arity`; when arity is 0, Elixir warns that `1..0` defaults to a negative step, and constructor argument generation is incorrect for a zero-arity relation.
Impact: Generated docs for user schemas are unpolished and potentially misleading. Zero-arity relations are an edge case, but if allowed by Program schemas, constructor generation should handle them correctly.
Recommended Fix: Build argument names with `Enum.map(1..arity//1, ...)` guarded for arity 0, or use `for i <- 1..arity, arity > 0` equivalent. Fix the doc string interpolation. Consider explicitly rejecting zero-field relations if they are not supported.
Suggested Test: Compile a schema with a normal relation and inspect `Code.fetch_docs/1` for the generated constructor doc. Add a zero-field relation test for either successful `rel()` construction or a clear compile-time error.
Confidence: Medium
```
```text
ID: L-002
Severity: Low
Category: Testing / Error Handling
Location: test/ex_datalog/runtime_facts_test.exs:153-162; lib/ex_datalog/program.ex:272-280
Summary: Program.add_facts/2 tests do not define partial-add semantics or malformed input behavior.
Evidence: The "stops on first error" test asserts only `{:error, _}`. It does not assert whether the earlier successful fact is retained/discarded, and malformed list elements would currently raise FunctionClauseError via add_fact/2 matching.
Impact: Public bulk-loading semantics are ambiguous. Users loading datasets need predictable behavior when one row is invalid.
Recommended Fix: Decide and document whether add_facts/2 is transactional (all-or-error) or partial (returns error after some adds). If transactional, validate all facts before adding. If partial, return richer error context. Handle malformed fact terms with `{:error, reason}`.
Suggested Test: Add tests for one invalid fact after a valid fact and for a malformed fact such as `:not_a_fact`. Assert exact behavior and error message.
Confidence: High
```
```text
ID: L-003
Severity: Low
Category: Documentation
Location: README.md:50, README.md:513, CHANGELOG.md:56
Summary: Test counts are stale after adding runtime facts tests.
Evidence: README and CHANGELOG still state 845 tests. Current suite output is 855 tests, 152 doctests, 10 properties, 0 failures.
Impact: Minor release metadata inconsistency.
Recommended Fix: Update counts after the final test run, or avoid exact test counts in user-facing docs to reduce churn.
Suggested Test: None needed; this is documentation maintenance.
Confidence: High
```
```text
ID: L-004
Severity: Low
Category: Benchmarks / Documentation
Location: bench/aggregate_bench.exs:69-82; bench/magic_sets_bench.exs; chest/benchmarks/v0.5.0-report.md
Summary: Benchmark scripts do not save results under bench/results despite the directory being kept.
Evidence: Both benchmark files use only `Benchee.Formatters.Console`; no formatter writes to bench/results. The report is manually written under chest/benchmarks. An empty bench/results directory is kept with .gitkeep.
Impact: Minor confusion for contributors expecting reproducible benchmark artifacts in bench/results.
Recommended Fix: Either remove bench/results/.gitkeep and document that reports are manually saved under chest/benchmarks, or add `benchee_json` / another formatter and write generated outputs there.
Suggested Test: Not applicable.
Confidence: High
```
```text
ID: N-001
Severity: Nit
Category: LLM Artifact / Style
Location: README.md; docs/articles/06_query_planning_in_datalog.md; docs/articles/07_aggregates_in_datalog.md; docs/articles/08_extending_datalog_with_beam_callbacks.md; docs/articles/09_magic_sets_and_demand_driven_evaluation.md; livebooks/ex_datalog_v050-v2.livemd
Summary: Some new docs show generated-documentation artifacts and marketing tone.
Evidence: README opens with "A production-grade Datalog engine" while v0.5.0 includes experimental magic sets. The articles are long and repetitive in places. Several files use long dashes heavily. The duplicate livebook title includes "DSL Edition - fork".
Impact: Does not affect behavior, but weakens polish and trust in release docs.
Recommended Fix: Tighten docs, remove duplicated/fork files, avoid unsupported marketing claims, and keep prose closer to existing project style.
Suggested Test: Human editorial pass; no automated test needed.
Confidence: Medium
```
Material healthy areas:
- Aggregate tests cover happy paths for `count`, `sum`, `min`, `max`, DSL aggregate syntax, filtering before grouping, and aggregate stratification.
- Callback tests cover boolean filters, value-returning callbacks, exceptions, timeouts, validation, and DSL `predicate/5` usage.
- Planner tests cover basic plan shape and `explain_plan/2` strategy rendering.
- Runtime facts tests cover relation constructors, `Schema.new/0`, `Program.add_fact/2`, `Program.add_facts/2`, and happy-path pipe materialization.
- Property tests cover path-style magic-sets result equivalence for randomly generated edge sets.
Important gaps:
1. **Magic sets public API path is not tested.** Existing tests cannot distinguish magic transform from post-filtering. Add telemetry/stats-backed tests that assert transform applied/fallback status through `ExDatalog.materialize/2`.
2. **Aggregate safety misses result-not-in-head.** Add builder and DSL tests for `count(E, N)` with head `dept(D)` and assert validation error.
3. **Aggregate type/runtime errors are not tested.** Add `sum/min/max` non-integer tests and define expected structured error behavior.
4. **Program.materialize error propagation is not tested.** Add failed pipe-chain test and assert `{:error, reason}`.
5. **Planner callback classification is not tested.** Add callback plan test.
6. **Runtime facts malformed input behavior is not tested.** Add malformed fact and partial bulk-load semantics tests.
7. **Docs examples are not exercised.** README and Livebook examples should have either doctests or mirrored executable examples.
Material issues:
- README v0.5 aggregate/callback examples use invalid DSL syntax and invalid materialization calls.
- Schema moduledoc still describes aggregates as unsupported and planned for v0.6.0.
- Schema generated docs reference `new_program/0`; the implemented API is `new/0`.
- Schema `predicate/5` docs claim compile-time callback validation; implementation validates during `materialize/compile` via `Validator`.
- BeamCallback moduledoc says callbacks run in a `Task`; implementation uses `spawn_monitor`.
- Planner docs say callbacks are included as predicates; implementation ignores callbacks.
- Livebook teaches static compile-time facts for data-heavy examples despite the newly introduced runtime facts API.
- Duplicate `livebooks/ex_datalog_v050-v2.livemd` contains invalid `DeptCount.fact(...)` API calls and should not be committed.
- README/CHANGELOG test counts are stale after the latest runtime-facts tests.
Areas that appear healthy:
- The migration guide broadly explains the intended v0.5.0 feature set and includes useful examples.
- Articles 06-09 cover the new conceptual areas in depth, though they need a polish pass for exact claims and concision.
- `mix docs --warnings-as-errors` passes, so there are no broken ExDoc references at build time after the current edits.
Findings:
- No emojis were found in reviewed new files.
- Long dashes and broad marketing phrases appear frequently in README/articles/livebook.
- `livebooks/ex_datalog_v050-v2.livemd` looks like an intermediate generated fork and contains invalid API examples.
- Some docs repeat the same framing language ("deterministic, side-effect-free", "goal-directed", "program transformation") more than needed.
- The volume of article prose is high relative to the maturity of the APIs, especially while public behavior still diverges from docs.
Recommendation: remove the fork file, tighten README and livebook to executable examples, and do one editorial pass on articles for concision and exactness.
Material findings:
- `livebooks/ex_datalog_v050-v2.livemd` is a duplicate/fork and should not be committed unless intentionally renamed and fixed.
- `bench/results/.gitkeep` keeps an empty directory, but benchmark scripts no longer write results there. This is not harmful, but it is currently unnecessary.
- Planner `Predicate` currently cannot represent callbacks despite docs claiming it does; this is more an incomplete implementation than dead code.
No material unused source modules were found among the new core modules. `benchee` is used by the benchmark scripts and is scoped to `:dev`.
1. Fix `ExDatalog.materialize/2` so `strategy: :magic_sets, goal: ...` actually passes the goal to the evaluator and runs `MagicSets.transform/2` for supported programs.
2. Add tests that prove the public magic-sets API uses the transform path, not just post-filtering.
3. Enforce aggregate result variable in rule head.
4. Enforce integer-only aggregate behavior or update docs if non-integer `min/max` are intentionally allowed.
5. Make `Program.materialize/1,2` propagate `{:error, reason}` in pipe chains.
6. Fix invalid README v0.5 examples.
7. Update Schema docs for aggregates, runtime facts (`new/0`), and callback validation timing.
8. Remove or fix `livebooks/ex_datalog_v050-v2.livemd` before commit.
1. Flush late callback result messages after timeout.
2. Include callback predicates in `Planner.plan/2`, or explicitly document that callbacks are excluded in v0.5.0.
3. Rewrite `livebooks/ex_datalog_v050.livemd` to use runtime facts via `Schema.new/0`, relation constructors, and `Program.add_facts/2`.
4. Add malformed input and partial-load semantics tests for `Program.add_facts/2`.
5. Fix generated relation constructor docs and decide zero-arity relation support.
6. Update stale test counts in README/CHANGELOG or remove exact counts.
1. Decide whether benchmark scripts should write machine-readable results under `bench/results`.
2. Tighten article prose and reduce generated-looking repetition.
3. Add executable docs/example testing for README and Livebook snippets.
**Needs Significant Fixes**
The branch has strong momentum and many tests pass, but the primary public magic-sets API does not actually execute the magic-sets strategy because `:goal` is stripped before evaluation. That is a release-blocking mismatch between advertised functionality and implementation. Aggregate validation/runtime behavior and pipe-friendly error propagation also need fixes before v0.5.0 should be tagged.
After the Must Fix items are addressed and tests prove the public API behavior, this can likely move to **Merge After Minor Fixes** quickly.
High — Correctness - F-001 (the real bug): MagicSets.rewrite_rule/5 now uses Enum.map_reduce to give each supplementary rule a distinct ID, and do_transform runs a defensive assert_unique_rule_ids!/1 on the transformed IR. Verified the doubly-recursive case now produces IDs [0,1,2,3] with no provenance-map collapse. - F-007: Added a doubly-recursive (reach(X,Z) :- reach(X,Y), reach(Y,Z)) regression suite — asserts unique IDs, full provenance map, correct goal subset, and a clean explain: true run. High — Documentation contradictions (introduced by the branch) - F-002 / F-013: Rewrote article 06 — the planner is now correctly described as a standalone inspection tool off the eval path; removed the nonexistent eval_plan/2; corrected the "magic sets still runs semi-naive" claim; fixed the "is now.Compiler" typo and all the "engine consumes the plan" statements. - F-003: Article 07 now documents the :aggregate_result_not_in_head rejection and that non-integer inputs raise ArgumentError. - F-004: Article 09 now shows the correct /5 and /4 signatures, documents the supplementary demand-propagation rule, and replaces the false "derives only four facts" claim with an accurate disconnected-component explanation. Medium - F-005: New tests for result-not-in-head rejection, sum/min/max integer guards raising, and count accepting non-integers. - F-006: Settled the contract — non-integer aggregate inputs raise (consistent with the integer-only design); documented in materialize/2 and the aggregate moduledoc, with tests pinning it. Low / Nit - F-008/F-009/F-010: Corrected stale docstrings (no build-time validation claim; sum raises not filters; updated the Enum.min/max reference). - F-011: flushlate → flush_late. - F-012: Zero-field relations now raise a clear DSL.CompileError at compile time instead of silently breaking the whole schema; documented.
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.
No description provided.