Skip to content

DEV-1788: unify aggregated slot-type and display-format inference - #307

Merged
ZmeiGorynych merged 2 commits into
egor/dev-1742-one-doctrine-consolidation-of-sql-generation-dev-1450from
egor/dev-1788-aggregated-slot-format-inference-diverges-from-slot-type-for
Aug 16, 2026
Merged

DEV-1788: unify aggregated slot-type and display-format inference#307
ZmeiGorynych merged 2 commits into
egor/dev-1742-one-doctrine-consolidation-of-sql-generation-dev-1450from
egor/dev-1788-aggregated-slot-format-inference-diverges-from-slot-type-for

Conversation

@ZmeiGorynych

@ZmeiGorynych ZmeiGorynych commented Aug 16, 2026

Copy link
Copy Markdown
Member

Follow-up to DEV-1784 (PR #286 review G2, item 8). Reconciles the two parallel inference functions that disagreed on stat/parametric aggregations.

Problem

aggregated_type (slot DataType) and _infer_aggregated_format (display NumberFormat) diverged on the stat/parametric family: type said DOUBLE while format fell through to inherit the source column's format, so revenue:stddev_samp was typed DOUBLE yet displayed as currency.

Fix

Both functions now read a single classify_aggregation (core/enums.py) returning one of four AggregationValueClass buckets; each maps the bucket to its own output, so no per-name branching survives and the two axes cannot drift. The four builtin frozensets partition BUILTIN_AGGREGATIONS (pinned by a completeness test). Custom/model-defined aggregations hit the PRESERVING fallback (inherit type & format), unchanged.

category aggregations slot type display format
COUNT count(_distinct/_approx), * INT INTEGER
PRESERVING sum, min, max, first, last source type (None if absent) source format (None if absent)
FLOAT_SOURCE_UNITS avg, median, weighted_avg, percentile, stddev_samp, stddev_pop DOUBLE source format, else FLOAT
FLOAT_PLAIN corr, var_samp, var_pop, covar_samp, covar_pop DOUBLE FLOAT

Behavioral change (format only; aggregated_type unchanged)

  • avg/median/weighted_avg of a formatted measure now inherit that format (was FLOAT); of an unformatted measure stay FLOAT.
  • corr/var*/covar* → FLOAT (were inherit).
  • stddev*/percentile unchanged (already inherited).

Tests

Drift-guard matrix extended to the full four-bucket set and routed through the public callers (measure_key_type / measure_key_format_description); partition-completeness test; missing-source-column and custom-aggregation cases; response-metadata assertions for the stat/parametric family. Full non-integration suite green; ruff clean.

Base: this PR targets the umbrella branch egor/dev-1742-... per the issue, not main.

Summary by CodeRabbit

  • Improvements

    • Improved aggregation result types and number formatting for averages, counts, correlations, variance, covariance, percentiles, and standard deviations.
    • Average-based results now preserve source units such as currency where applicable.
    • Added consistent fallback formatting for fractional results without a source format.
    • Custom aggregations continue to preserve existing behavior.
  • Documentation

    • Expanded documentation describing aggregation type and format inference behavior.

aggregated_type and _infer_aggregated_format now share one
classify_aggregation classifier (core/enums.py) returning a 4-bucket
AggregationValueClass, so slot type and display format cannot drift.

Behavioral (format only): avg/median/weighted_avg of a formatted measure
inherit its format (was FLOAT); corr/var*/covar* -> FLOAT (were inherit);
stddev*/percentile unchanged. aggregated_type unchanged.
@linear

linear Bot commented Aug 16, 2026

Copy link
Copy Markdown

DEV-1788

@coderabbitai

coderabbitai Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 20 seconds

Limit details: You’ve used all 3 included reviews currently available under your plan. You completed 41 included PR reviews in the past 7 days; at that activity level, included reviews refill at 3 reviews per hour.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3a1f1db1-a862-4921-9f55-904bc875705a

📥 Commits

Reviewing files that changed from the base of the PR and between 03d894d and 8ead7ce.

📒 Files selected for processing (1)
  • DECISIONS.md

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 6bfed547-08dc-413f-b2ea-6f9758ef171a

📥 Commits

Reviewing files that changed from the base of the PR and between db3428e and 03d894d.

📒 Files selected for processing (7)
  • DECISIONS.md
  • docs/architecture/sql-generation.md
  • slayer/core/enums.py
  • slayer/engine/prebound.py
  • slayer/engine/response_meta.py
  • tests/test_format_propagation.py
  • tests/test_response_meta.py

Included review availability: 0 reviews are currently available. Based on recent review activity, included reviews refill at 3 per hour.


📝 Walkthrough

Walkthrough

The change adds a shared aggregation classifier. Prebound result types and response metadata formats now use four aggregation classes. Tests cover source-format inheritance, float fallbacks, custom aggregations, star counts, missing columns, and caller consistency.

Changes

Aggregation inference

Layer / File(s) Summary
Aggregation result classification
slayer/core/enums.py
Adds four aggregation classes and classify_aggregation for built-in, wildcard, custom, and unknown aggregations.
Type and format inference integration
slayer/engine/prebound.py, slayer/engine/response_meta.py, docs/architecture/sql-generation.md, DECISIONS.md
Uses shared classification for result types and formats. Count operations use integer results. Plain-float operations use FLOAT. Source-unit and preserving operations inherit source formats where applicable.
Propagation and metadata validation
tests/test_format_propagation.py, tests/test_response_meta.py
Adds coverage for all classification buckets, source formats, fallbacks, missing columns, custom aggregations, star counts, and public caller consistency.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 03d89

The PR unifies aggregated type and display-format inference with focused tests and no actionable merge-blocking risk remains beyond normal checks and review.

Possibly related PRs

  • MotleyAI/slayer#258: Shares aggregation classification and result type/format inference changes.
  • MotleyAI/slayer#302: Introduces related shared integer-aggregation logic extended by this change.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 3.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: unifying aggregated slot-type and display-format inference.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch egor/dev-1788-aggregated-slot-format-inference-diverges-from-slot-type-for

Comment @coderabbitai help to get the list of available commands.

…lidation-of-sql-generation-dev-1450' into egor/dev-1788-aggregated-slot-format-inference-diverges-from-slot-type-for

# Conflicts:
#	DECISIONS.md
@sonarqubecloud

Copy link
Copy Markdown

@ZmeiGorynych
ZmeiGorynych merged commit 7f6e90d into egor/dev-1742-one-doctrine-consolidation-of-sql-generation-dev-1450 Aug 16, 2026
6 checks passed
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