DEV-1564: per-dialect integration test CI for MySQL, ClickHouse, SQL Server#184
Merged
ZmeiGorynych merged 5 commits intoJun 15, 2026
Conversation
…Server Stand up testcontainers-driven pytest integration suites for the three Tier-1 dialects that previously had only verify.py-based CI (or, for SQL Server, no CI coverage at all). Each suite mirrors the Postgres suite's coverage and adds dialect-specific extras: parametric quantile() and DateTime type ingestion for ClickHouse; variance-decomposition SQL shape and canonical VAR_SAMP/VAR_POP names for MySQL; DATETRUNC(iso_week, ...) and DATEADD for SQL Server, plus the variance-decomposition path with T-SQL names (VAR/VARP/STDEV). median/percentile raise NotImplementedError on MySQL and SQL Server; ClickHouse uses native median(x). Each dialect owns one workflow at .github/workflows/integration-<dialect>.yml, path-gated to the dialect file + shared SQL generator + dialect base + example dir + test file + the workflow itself, plus workflow_dispatch. The SQL Server workflow installs msodbcsql18 on the runner because pyodbc imports in-process. The previous integration-examples matrix in ci.yml is removed; verify.py runs now live inside the per-dialect workflows so each dialect's CI is in one place. ci.yml's always-on integration step --ignores the three new dialect suites so they don't run unconditionally on every PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
More reviews will be available in 2 hours, 47 minutes, and 25 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ 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 ignored due to path filters (1)
📒 Files selected for processing (10)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR review pass:
- xfail 8 tests catching pre-existing SLayer T-SQL / MySQL SQL generator
bugs, tracked in DEV-1571. Each xfail carries `strict=True` so a fix in
follow-up PRs auto-promotes the test from XFAIL to PASSED.
* 4 SQL Server CTE-in-derived-table tests (time_shift_with_date_range,
change_with_date_range, change_pct_with_date_range,
multiple_date_range_shifts).
* 1 SQL Server bracket-aliased ORDER BY test
(test_integration_sqlserver_cross_model_derived_columnsql).
* 3 MySQL outer-wrap quote-style-mismatch tests
(change_pct_with_date_range, multiple_date_range_shifts,
test_cross_model_measure).
- ClickHouse test_median_emits_native_function → rename to
test_median_emits_native_aggregate, accept both `quantile(0.5)(x)` (what
sqlglot's ClickHouse dialect emits for native median()) and `median(x)`.
Block the percentile_cont fallback regression.
- Codex F1: SQL Server CI's apt repo URL was hardcoded to Ubuntu 22.04;
derive from /etc/os-release so it tracks `ubuntu-latest` as the runner
image rolls forward.
- Codex F2: broaden path filters in all 3 new dialect workflows to also
trigger on slayer/{sql,engine,core,storage}/** + pyproject.toml +
poetry.lock. The main ci.yml --ignores these suites, so without this,
shared-runtime changes would land with no MySQL/CH/SQL Server CI signal.
- Sonar: NOSONAR(S2068)/NOSONAR(S6437,S2068) on the 6 testcontainer
password lines; NOSONAR(S1244) on the 10 integer-cent sum-equality
assertions; rename TestRollupIngestion_<Dialect> →
TestRollupIngestion<Dialect> (S101); drop the 2 duplicated
test_dotted_dimension_single_hop methods (S4144); drop unnecessary
list() wraps around dict.values() iteration (S7504).
- New sonar-project.properties pins the project key + organization and
adds the 3 new test files to sonar.cpd.exclusions — the CPD rule has
no per-line NOSONAR form and the by-design Postgres-mirror duplication
isn't worth a refactor that would tangle dialect concerns.
DEV-1571 owns the actual SLayer generator fixes; it's branch-only for
now, no PR yet.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…h filters Round-2 review pass: - xfail TestSQLServerQueries::test_consecutive_periods_with_boolean_predicate with the same DEV-1571 Bug 1 reason (CTE-in-derived-table on T-SQL). It was missed in round 1 because the earlier CI log was truncated at the 5th failure; DEV-1571's affected-tests list also updated to include it. - Codex F1: broaden the path filter on all 3 per-dialect workflows with slayer/api/**, slayer/mcp/**, and slayer/cli.py so the verify-example jobs (which boot the full HTTP stack via `slayer serve`) catch regressions in server-startup code. Unit tests cover the API/CLI paths at unit level, but integration breakage that survives unit-level CI would otherwise reach main with no dialect signal. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ters
Round 2 broadened path filters with slayer/api/**, slayer/mcp/**,
slayer/cli.py (Codex F1). Round 1 had already broadened with slayer/sql/**,
slayer/engine/**, slayer/core/**, slayer/storage/**, pyproject.toml,
poetry.lock (Codex F2). Both drifted from the user's original spec choice
("Dialect-specific files + SQL generator core" — Option 2 from the
AskUserQuestion in the planning phase).
Reverting to the originally-chosen narrow set:
- slayer/sql/dialects/<dialect>.py
- slayer/sql/dialects/base.py
- slayer/sql/generator.py
- examples/<dialect>/**
- tests/integration/test_integration_<dialect>.py
- .github/workflows/integration-<dialect>.yml
Reviewer findings that propose expanding the scope of a decision the user
already made should be classified INVALID by default. The xfail addition
from round 2 (test_consecutive_periods_with_boolean_predicate) stays.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous round's MySQL xfail set caught the cross-model + 2 date-range shift cases, but missed three more variants of the same outer-wrap quote-style mismatch: - test_time_shift_with_date_range (loud — pymysql 1064) - test_change_with_date_range (loud — pymysql 1064) - test_consecutive_periods_with_boolean_predicate (silent — MySQL parses the outer-wrap's "orders.positive_run" as a string literal and the result returns the literal column-name text on every row instead of the underlying value) The silent variant is the more interesting one — no syntax error, just wrong data returned to the caller. DEV-1571 Bug 3 updated to document both the loud and silent failure modes plus the new affected tests, and to require a per-dialect identifier-quoting unit test as part of the eventual fix so future regressions can't silently flow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
5 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
Closes DEV-1564.
tests/integration/test_integration_{mysql,clickhouse,sqlserver}.py— testcontainers-driven, full mirrors oftest_integration_postgres.pyplus dialect-specific extras (~135 tests total)..github/workflows/integration-{mysql,clickhouse,sqlserver}.yml, each running two parallel jobs —pytest(testcontainers) andverify-example(existing docker-compose + verify.py). Path-gated toslayer/sql/dialects/<dialect>.py,slayer/sql/dialects/base.py,slayer/sql/generator.py,examples/<dialect>/**, the test file, and the workflow itself. All exposeworkflow_dispatch.msodbcsql18on the runner because pyodbc imports IN-PROCESS.ci.yml'sintegration-examplesmatrix is removed (migrated into the per-dialect workflows). The always-onpytest -m integrationstep--ignores the three new files so they don't run unconditionally.pyproject.toml:testcontainers[mysql,clickhouse,mssql] ^4.0added to the dev group;poetry.lockregenerated.CLAUDE.md: per-dialect CI convention documented under Database Support.Dialect-specific coverage highlights
VAR_SAMP,STDDEV_SAMP). VAR_SAMP/VAR_POPexp.Anonymousoverrides pinned via dry_run. Native LOG10 round-trip.median(x)(ungrouped + grouped) + parametricquantile(p)(x)(dry_run SQL shape). Native stddev/var/corr/covar with tight numeric tolerance. Native LOG10 round-trip. DateTime →DataType.TIMESTAMPingestion regression (issue ClickHouse columns ingested as STRING — _SA_TYPE_MAP misses Int32/Int64/Float64 etc. #62).Notable design choices
engine_factory._engine_cache), thenALTER DATABASE ... SET SINGLE_USER WITH ROLLBACK IMMEDIATEbeforeDROP DATABASEso pyodbc connection pools don't block the drop.test_log10_round_tripuses a dedicated function-scoped fixture (not the shared module-scoped storage) so its Column-add doesn't leak into sibling tests.python -c "import testcontainers.<dialect>"before pytest so a missing extra surfaces as workflow failure, not a silentimportorskipskip.Test plan
poetry run ruff check slayer/ tests/— clean.poetry run pytest --collect-onlyon the three new files — 135 tests collect.poetry run pytest -m "not integration" --timeout=120— 4614 passed, 1 pre-existing flake (test_cli_argparse_subprocess_accepts_description_flag, fails identically on clean HEAD; subprocess+litellm timing unrelated to this work).ubuntu-latest).🤖 Generated with Claude Code