DEV-1566: pg-facade — CAST(<column> AS <type>) in projection#185
Conversation
Pattern-match CAST around a bare or qualified Column in the shared facade translator. The engine still executes the bare column; the wire OID is overridden via projection_types, made safe by a strict per-pair coercion allowlist. Flips the DEV-1562 strict-xfail test_timestamp_round_trip_both_formats. Codex round-1 follow-ups folded in: OID_TIMESTAMP + dt.date widening in value_to_text (text-format DATE→TIMESTAMP was emitting bare YYYY-MM-DD); OID_TEXT + bool now emits true/false (Postgres text shape) rather than t/f; _order_by_name and _validate_group_by gain CAST detection branches so the canonical lowercase form resolves; DATE / BOOLEAN columns added to the translator test fixture; DOUBLE→INT dropped from the allowlist (Python int(x) truncates while Postgres rounds half-to-even); unknown source data type admits TEXT only. Codex round-2 follow-ups: column_name_mapping for aliased CAST now points at the bare engine column (engine still executes it); type-alias coverage for VARCHAR/INTEGER/BIGINT/SMALLINT/FLOAT/REAL/DECIMAL/DATETIME/TIMESTAMPTZ; aliased + qualified-ref ORDER BY / GROUP BY canonical resolution; full error-contract pinning; non-aggregate non-column inner rejection. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughImplements ChangesCAST Projection Support (DEV-1566)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
slayer/facade/translator.py (1)
1421-1423: ⚡ Quick winUse keyword arguments for
_alias_for_column_castcalls.These new call sites use positional arguments on a 2-parameter function; switch to keyword arguments to match repo rules.
Proposed diff
- return _alias_for_column_cast( - _column_to_dotted(inner_col, strip_prefix=strip_prefix), target, - ) + return _alias_for_column_cast( + dotted=_column_to_dotted(inner_col, strip_prefix=strip_prefix), + target=target, + ) ... - user_items.append(_alias_for_column_cast( - _column_to_dotted(inner_col, strip_prefix=strip_prefix), - target, - )) + user_items.append(_alias_for_column_cast( + dotted=_column_to_dotted(inner_col, strip_prefix=strip_prefix), + target=target, + )) ... - item_by_projected_name.setdefault( - _alias_for_column_cast(dotted, item.cast_target), item, - ) + item_by_projected_name.setdefault( + _alias_for_column_cast(dotted=dotted, target=item.cast_target), item, + )As per coding guidelines,
**/*.py: “Use keyword arguments for functions with more than 1 parameter.”Also applies to: 1454-1457, 1781-1783
🤖 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 `@slayer/facade/translator.py` around lines 1421 - 1423, The _alias_for_column_cast function calls use positional arguments instead of keyword arguments, which violates the repository's coding guidelines requiring keyword arguments for functions with more than one parameter. In slayer/facade/translator.py at lines 1421-1423 (the anchor location), convert the positional arguments in the _alias_for_column_cast call to use explicit keyword argument names. Apply the same conversion at the two sibling locations: lines 1454-1457 and lines 1781-1783, ensuring all three call sites use keyword arguments to match repository conventions.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.
Nitpick comments:
In `@slayer/facade/translator.py`:
- Around line 1421-1423: The _alias_for_column_cast function calls use
positional arguments instead of keyword arguments, which violates the
repository's coding guidelines requiring keyword arguments for functions with
more than one parameter. In slayer/facade/translator.py at lines 1421-1423 (the
anchor location), convert the positional arguments in the _alias_for_column_cast
call to use explicit keyword argument names. Apply the same conversion at the
two sibling locations: lines 1454-1457 and lines 1781-1783, ensuring all three
call sites use keyword arguments to match repository conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4dac113f-597d-4190-949f-68bf290a1e5d
📒 Files selected for processing (7)
docs/interfaces/pg-facade.mdslayer/facade/translator.pyslayer/pg_facade/types.pytests/facade/test_translator.pytests/integration/test_integration_pg_facade.pytests/integration/test_metabase_e2e.pytests/pg_facade/test_types.py
💤 Files with no reviewable changes (1)
- tests/integration/test_metabase_e2e.py



Summary
CAST(<column> AS <type>)(andcol::typesugar) in the shared facade translator; the engine still executes the bare column and the wire OID is overridden viaprojection_types. Strict per-pair coercion allowlist gates which (source, target) combos round-trip cleanly.slayer/pg_facade/types.py: OID_TIMESTAMP +dt.datewidening (text-format DATE→TIMESTAMP was emitting bareYYYY-MM-DD); OID_TEXT +boolnow emitstrue/false(Postgres text shape) instead oft/f.tests/integration/test_metabase_e2e.py::test_timestamp_round_trip_both_formatsfrom PR DEV-1562: live-Metabase end-to-end pg-facade integration test suite #182's suite.Base is
egor/dev-1562-...(PR #182) because that branch carriestest_metabase_e2e.py; will re-target main once #182 lands.Two rounds of Codex review folded in pre-PR. Decisions documented inline in the commit message.
Test plan
poetry run pytest -m \"not integration\"clean — verified locally: 4630 pass, 1 unrelated flake (test_save_memory_description_plumbing.py::test_cli_argparse_subprocess_accepts_description_flag, passes in isolation)poetry run ruff check slayer/ tests/clean — verifiedpoetry run pytest tests/integration/test_integration_pg_facade.py -m integration(new rejected-coercion case)test_timestamp_round_trip_both_formatspasses once DEV-1562: live-Metabase end-to-end pg-facade integration test suite #182 merges and this rebases onto maindatetime.datetimeforSELECT CAST(ordered_at AS TIMESTAMP) FROM orders LIMIT 1🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features