Add live Redshift connector (driver_map + redshift extra) - #284
Open
aalttoaesto wants to merge 4 commits into
Open
Add live Redshift connector (driver_map + redshift extra)#284aalttoaesto wants to merge 4 commits into
aalttoaesto wants to merge 4 commits into
Conversation
RedshiftDialect's SQL generation has been code-covered (Tier 2) since DEV-1542, but DatasourceConfig.get_connection_string() had no explicit driver_map entry for it, silently depending on sqlalchemy-redshift's bare "redshift" scheme (psycopg2, password-auth only) rather than the documented redshift-connector driver. Wire it explicitly to redshift+redshift_connector (IAM/Serverless/SSO-capable), add the `redshift` extra, and scaffold a live integration suite that skips cleanly without credentials — same pattern as Snowflake/BigQuery before their CI secrets existed. Not yet run against a live cluster; stays Tier 2 in docs/database-support.md until the new redshift-integration CI job passes at least once against a real endpoint. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughRedshift support now includes optional dependencies, explicit connection mapping, live integration tests, an example verifier, configuration documentation, database-support status, and a gated CI job. ChangesRedshift support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Pytest
participant RedshiftFixtures
participant Redshift
participant SlayerQueryEngine
Pytest->>RedshiftFixtures: Create transient schema and seed data
RedshiftFixtures->>Redshift: Execute setup SQL
Pytest->>SlayerQueryEngine: Execute integration query
SlayerQueryEngine->>Redshift: Run generated SQL
Redshift-->>SlayerQueryEngine: Return query results
SlayerQueryEngine-->>Pytest: Return validated results
Pytest->>RedshiftFixtures: Drop schema and close resources
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
tests/integration/test_integration_redshift.py (1)
32-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffKeep imports in the module import section.
Move optional-dependency collection gating out of this module, then keep imports contiguous at the module top. Lines 33-34 execute before the imports on lines 36-49. The
noqa: E402annotations suppress the required import layout.As per coding guidelines, "Keep imports at the top of files."
🤖 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_integration_redshift.py` around lines 32 - 49, Move the optional Redshift dependency gating out of the module-level import sequence, and keep all imports in the module’s top import section. Remove the E402 suppressions from the imports around redshift_connector and the Slayer symbols, while preserving the existing collection-level skip behavior through the project’s supported configuration mechanism.Source: Coding guidelines
examples/redshift/verify.py (3)
32-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPass the
checkarguments by keyword.Line 32 passes two positional arguments. Use the parameter names declared by
checkinexamples/verify_common.py.As per coding guidelines, use keyword arguments for functions with more than one parameter.
🤖 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 `@examples/redshift/verify.py` at line 32, Update the check call in the Redshift verification flow to pass both arguments using the parameter names declared by check in verify_common.py, rather than positional arguments; preserve the existing message and validation expression.Source: Coding guidelines
3-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRun the example through Poetry.
Lines 4-6 invoke project commands outside the Poetry environment. Use
poetry run pythonfor the Python commands andpoetry run slayerfor the CLI command.Proposed change
- python examples/seed.py "redshift+redshift_connector://user:pass@host:5439/dev" - slayer datasources create "redshift+redshift_connector://user:pass@host:5439/dev" --name rs --ingest - python examples/redshift/verify.py + poetry run python examples/seed.py "redshift+redshift_connector://user:pass@host:5439/dev" + poetry run slayer datasources create "redshift+redshift_connector://user:pass@host:5439/dev" --name rs --ingest + poetry run python examples/redshift/verify.pyAs per coding guidelines, use
poetry runfor all Python commands.🤖 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 `@examples/redshift/verify.py` around lines 3 - 6, Update the example commands in the usage block to run every Python script via “poetry run python” and the Slayer CLI via “poetry run slayer”, preserving the existing arguments and command order.Source: Coding guidelines
18-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep imports at the top of the file.
Line 18 executes
sys.path.insert(...)before the module-level import on Line 19. Use an importable package/module entry point soverify_commoncan be imported with the other top-level imports.As per coding guidelines, keep imports at the top of files.
🤖 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 `@examples/redshift/verify.py` around lines 18 - 19, Move the verify_common import setup out of the executable sys.path.insert call in the module body; update the import to use the project’s importable package/module entry point so all imports remain together at the top of verify.py. Remove the path mutation and preserve the existing verify_common symbols consumed by the script.Source: Coding guidelines
🤖 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.
Inline comments:
In `@docs/configuration/datasources.md`:
- Around line 157-180: Update the Redshift configuration documentation near the
alternate redshift+psycopg2 connection string to explicitly state that
psycopg2-binary is not included in the redshift extra and must be installed
separately (or document the corresponding extra dependency). Keep the existing
structured redshift-connector guidance unchanged.
In `@examples/redshift/README.md`:
- Around line 35-37: Update the fenced connection-string block in the README to
specify the text language tag, changing the opening fence before the redshift
connection string to use text while preserving the content and closing fence.
In `@tests/integration/test_integration_redshift.py`:
- Around line 312-330: The test
test_approx_count_distinct_uses_approximate_keyword must validate SQL shape as
well as the result. Capture the generated statement through the engine’s dry-run
or statement-generation path, assert it contains “APPROXIMATE COUNT(DISTINCT”,
then retain the existing result-value assertion.
---
Nitpick comments:
In `@examples/redshift/verify.py`:
- Line 32: Update the check call in the Redshift verification flow to pass both
arguments using the parameter names declared by check in verify_common.py,
rather than positional arguments; preserve the existing message and validation
expression.
- Around line 3-6: Update the example commands in the usage block to run every
Python script via “poetry run python” and the Slayer CLI via “poetry run
slayer”, preserving the existing arguments and command order.
- Around line 18-19: Move the verify_common import setup out of the executable
sys.path.insert call in the module body; update the import to use the project’s
importable package/module entry point so all imports remain together at the top
of verify.py. Remove the path mutation and preserve the existing verify_common
symbols consumed by the script.
In `@tests/integration/test_integration_redshift.py`:
- Around line 32-49: Move the optional Redshift dependency gating out of the
module-level import sequence, and keep all imports in the module’s top import
section. Remove the E402 suppressions from the imports around redshift_connector
and the Slayer symbols, while preserving the existing collection-level skip
behavior through the project’s supported configuration mechanism.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: aec3d08b-12c6-4e1d-ab13-eb490bbcb58b
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.github/workflows/ci.ymlDECISIONS.mddocs/configuration/datasources.mddocs/database-support.mdexamples/redshift/README.mdexamples/redshift/verify.pypyproject.tomlslayer/core/models.pytests/integration/test_integration_redshift.pytests/test_models.py
aalttoaesto
changed the title
aalttoaesto
marked this pull request as draft
August 5, 2026 19:15
# Conflicts: # DECISIONS.md # poetry.lock
|
aalttoaesto
marked this pull request as ready for review
August 5, 2026 19:33
aalttoaesto
changed the title
Member
|
@aalttoaesto would you like me to polish this up so it can be merged? |
Author
|
@ZmeiGorynych hey, yes please, appreciate it. |
whimo
reviewed
Aug 18, 2026
Comment on lines
+880
to
+890
| # Explicit rather than falling through to the bare "redshift" | ||
| # scheme (which sqlalchemy-redshift also registers, defaulting to | ||
| # its psycopg2 dialect): redshift-connector is AWS's maintained | ||
| # driver and the one this DBAPI-agnostic sqlalchemy-redshift | ||
| # setup is documented against (IAM auth, Redshift Serverless, | ||
| # browser SSO — psycopg2 only does password auth). Being | ||
| # explicit here also stops a future edit from "simplifying" this | ||
| # to "postgresql" — Redshift's wire protocol is Postgres-derived | ||
| # but its dialect (RedshiftDialect) carries real SQL differences | ||
| # (e.g. APPROXIMATE COUNT(DISTINCT)) that the bare postgresql | ||
| # dialect doesn't know about. |
Member
There was a problem hiding this comment.
This needs to be removed – there's no reason to put this whole rationale inline in the code.
whimo
reviewed
Aug 18, 2026
Comment on lines
+1
to
+25
| """Live integration tests for Redshift. | ||
|
|
||
| Skip-by-default: requires ``sqlalchemy-redshift`` + ``redshift-connector`` | ||
| (the ``motley-slayer[redshift]`` extra) and a real Redshift cluster/serverless | ||
| endpoint reachable via env vars: | ||
|
|
||
| REDSHIFT_HOST (required — cluster/serverless endpoint, no port) | ||
| REDSHIFT_PORT (optional, default 5439) | ||
| REDSHIFT_DATABASE (required) | ||
| REDSHIFT_USER (required) | ||
| REDSHIFT_PASSWORD (required) | ||
|
|
||
| Redshift has no free local Docker image (unlike Postgres/MySQL/ClickHouse/ | ||
| SQL Server) and no shared local-profile convention (unlike Snowflake's | ||
| ``~/.snowflake/connections.toml``), so — like BigQuery — this suite is | ||
| env-var-gated rather than file-gated, and skips cleanly in any environment | ||
| without those set (including plain `pytest` runs in this repo today). | ||
|
|
||
| Unlike the Snowflake suite, this does NOT assert on auto-ingestion's FK-join | ||
| discovery: Redshift allows declaring (unenforced) FOREIGN KEY constraints, | ||
| but whether SQLAlchemy's Inspector surfaces them through | ||
| ``sqlalchemy-redshift`` for auto-ingestion to pick up is unverified absent a | ||
| live cluster. Models below define ``joins=`` explicitly instead, which | ||
| exercises the query/rollup path regardless of that answer. | ||
| """ |
whimo
reviewed
Aug 18, 2026
Comment on lines
+82
to
+87
| # Redshift support. sqlalchemy-redshift has no hard DBAPI dependency of its | ||
| # own — it lazily imports whichever driver the connection URL's scheme | ||
| # selects. redshift-connector is AWS's actively-maintained driver (IAM auth, | ||
| # Redshift Serverless, browser-based SSO); we default the `redshift` type to | ||
| # it (see `DatasourceConfig.get_connection_string`) rather than the | ||
| # psycopg2-only `redshift://` form. |
whimo
reviewed
Aug 18, 2026
Comment on lines
+35
to
+51
| Trino/Presto (Athena uses the Presto dialect), Databricks/Spark, Oracle. | ||
|
|
||
| **Redshift** is further along than the rest of this tier: `RedshiftDialect` | ||
| (SQL generation) has been code-covered here for a while, and the connection | ||
| layer — `driver_map["redshift"] = "redshift+redshift_connector"`, the | ||
| `motley-slayer[redshift]` extra, and a live integration suite | ||
| (`tests/integration/test_integration_redshift.py`, gated on `REDSHIFT_HOST` / | ||
| `REDSHIFT_DATABASE` / `REDSHIFT_USER` / `REDSHIFT_PASSWORD`) — now exist too. | ||
| The suite has passed manually against a live Redshift Serverless endpoint | ||
| (outside CI). It stays in Tier 2 rather than Tier 1 because the | ||
| `redshift-integration` CI job (`.github/workflows/ci.yml`) has not yet run it | ||
| against a live endpoint — CI secrets are deliberately not wired up yet, since | ||
| the only endpoint verified so far is a personal AWS resource, and pointing CI | ||
| at it would bill every contributor's CI run to that personal account. This | ||
| entry should move to Tier 1 once the CI job passes at least once against a | ||
| project-owned (non-personal) Redshift endpoint, the same skip-until-configured | ||
| way Snowflake/BigQuery started. |
Member
There was a problem hiding this comment.
I would revert this: the fact that it has been manually tested once does not guarantee anything
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
3 participants
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
redshift+redshift_connectordriver fortype: redshift, replacing the implicit fallback tosqlalchemy-redshift's bareredshift://scheme (psycopg2, password-auth only).redshift-connectoris AWS's maintained driver and supports IAM auth / Redshift Serverless / browser SSO.redshiftextra (sqlalchemy-redshift+redshift-connector), a live integration suite (tests/integration/test_integration_redshift.py, env-var gated, skips cleanly without credentials), aredshift-integrationCI job (same skip-until-configured pattern as Snowflake/BigQuery), and a runnable example (examples/redshift/).docs/database-support.mdanddocs/configuration/datasources.mdwith the new connection details and current verification status.Verification
ruff check slayer/ tests/— clean.pytest -m "not integration") — 6588 passed, 0 failed.APPROXIMATE COUNT(DISTINCT)dialect quirk).Staying Tier 2 for now
Redshift stays in Tier 2 rather than moving to Tier 1. Tier 1 means CI-enforced non-regression, and no CI job exercises this yet — the manual pass above proves the connector works today but doesn't protect against a future regression the way the other Tier 1 dialects' CI jobs do.
REDSHIFT_HOST/PORT/DATABASE/USER/PASSWORDare deliberately not wired into repo secrets yet: the only endpoint verified so far is a personal one, and the job's fork-PR skip doesn't gate same-repo pushes/PRs — pointing CI at it would bill every contributor's CI run to a personal AWS account and expose the credential to anything running in that job. This moves to Tier 1 once theredshift-integrationjob passes against a project-owned endpoint.Known gaps
sqlalchemy-redshift's Inspector surfaces them is unconfirmed). Treated like BigQuery/ClickHouse for now — declarejoins:manually.Test plan
ruff check slayer/ tests/pytest -m "not integration"(full unit suite)pytest tests/integration/test_integration_redshift.py -m integrationagainst a live Redshift Serverless endpointredshift-integrationCI job (pending a project-owned endpoint + secrets)Summary by CodeRabbit
New Features
Documentation
Tests