Skip to content

Conversation

@GuyEshdat
Copy link
Contributor

@GuyEshdat GuyEshdat commented Dec 10, 2025

…hift started to refuse these statements

Summary by CodeRabbit

  • New Features
    • Added support for Redshift database platform, expanding compatibility for test data management operations across additional database environments.

✏️ Tip: You can customize this high-level summary in your review settings.

@github-actions
Copy link
Contributor

👋 @GuyEshdat
Thank you for raising your pull request.
Please make sure to add tests and document all user-facing changes.
You can do this by editing the docs files in the elementary repository.

@coderabbitai
Copy link

coderabbitai bot commented Dec 10, 2025

Walkthrough

A new dialect-specific macro for Redshift (redshift__get_clean_elementary_test_tables_queries) was added to support cleaning elementary test tables. The macro delegates to an existing transactionless query generator, following the established pattern used by other database dialects.

Changes

Cohort / File(s) Change Summary
Redshift dialect support
macros/edr/tests/test_utils/clean_elementary_test_tables.sql
Added redshift__get_clean_elementary_test_tables_queries(test_table_relations) macro that invokes the transactionless clean elementary test tables query generator

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Straightforward macro addition following an established pattern from other dialects
  • Single file modification with no complex logic or conditional branching
  • Minimal scope—introduces only a delegation to an existing utility function

Poem

🐰 A whisker-twitching tale for Redshift's gain,
New macros sweep the test-table terrain,
With transactionless grace, so clean and bright,
Elementary tests now shine with light!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding Redshift support for non-transactional deletion of test temporary tables to address Redshift's rejection of transactional deletes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch redshift-remove-transcation-when-cleaning-test-temp-tables

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 8d91cf6 and 1011990.

📒 Files selected for processing (1)
  • macros/edr/tests/test_utils/clean_elementary_test_tables.sql (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: haritamar
Repo: elementary-data/dbt-data-reliability PR: 825
File: macros/utils/cross_db_utils/incremental_strategy.sql:13-15
Timestamp: 2025-10-05T16:25:01.719Z
Learning: In macros/utils/cross_db_utils/incremental_strategy.sql, the get_default_incremental_strategy() macro and its adapter-specific variants are used exclusively for Elementary's internal dbt artifact tables, all of which have unique keys defined. Therefore, defaulting to "merge" strategy for Redshift (and other adapters) is safe in this context.
Learnt from: GuyEshdat
Repo: elementary-data/dbt-data-reliability PR: 838
File: macros/utils/table_operations/delete_and_insert.sql:138-146
Timestamp: 2025-08-10T11:29:06.982Z
Learning: In the Elementary dbt-data-reliability codebase, the team prefers to add the `escape_reserved_keywords` macro only in places where they've actually encountered errors with reserved keywords, rather than preemptively adding it everywhere. They rely on E2E tests to catch future issues if reserved keywords cause problems in untested code paths. This pragmatic approach avoids unnecessary defensive programming and keeps the codebase simpler.
Learnt from: GuyEshdat
Repo: elementary-data/dbt-data-reliability PR: 838
File: macros/utils/table_operations/insert_rows.sql:156-0
Timestamp: 2025-08-10T11:28:43.632Z
Learning: In the Elementary dbt-data-reliability codebase, the `dremio__escape_special_chars` macro in `macros/utils/table_operations/insert_rows.sql` receives strings where single quotes are already escaped as `\'` from upstream processing. The macro correctly converts these to Dremio's SQL escaping format by replacing `\'` with `''`. This is different from other database implementations that handle bare single quotes directly.
Learnt from: GuyEshdat
Repo: elementary-data/dbt-data-reliability PR: 838
File: macros/utils/sql_utils/escape_reserved_keywords.sql:29-31
Timestamp: 2025-08-10T11:29:44.525Z
Learning: In the Elementary dbt-data-reliability codebase, the `escape_keywords` macro in `macros/utils/sql_utils/escape_reserved_keywords.sql` is specifically designed to escape SQL reserved keywords for different database adapters. Reserved keywords are predefined language tokens (like 'filter', 'sql', 'timestamp', 'value', 'one', 'min', 'max', 'sum', 'count' for Dremio) that don't contain special characters like double quotes. Therefore, the `dremio__escape_keywords` macro correctly wraps these keywords in double quotes without needing to handle embedded quotes, as reserved keywords by definition are simple identifiers.
Learnt from: GuyEshdat
Repo: elementary-data/dbt-data-reliability PR: 838
File: macros/utils/sql_utils/escape_reserved_keywords.sql:29-31
Timestamp: 2025-08-10T11:29:44.525Z
Learning: In the Elementary dbt-data-reliability codebase, the `escape_keywords` macro in `macros/utils/sql_utils/escape_reserved_keywords.sql` is specifically designed to escape SQL reserved keywords for different database adapters. Reserved keywords are predefined language tokens (like 'filter', 'sql', 'timestamp', 'value', 'one', 'min', 'max', 'sum', 'count' for Dremio) that don't contain special characters like double quotes. Therefore, the `dremio__escape_keywords` macro correctly wraps these keywords in double quotes without needing to handle embedded quotes, as reserved keywords by definition are simple identifiers.
Learnt from: haritamar
Repo: elementary-data/dbt-data-reliability PR: 825
File: macros/utils/table_operations/create_table_as.sql:0-0
Timestamp: 2025-07-22T15:41:57.338Z
Learning: The dbt.create_table_as macro breaks in dbt-fusion when not run from a materialization context, requiring custom implementations for table creation functionality in dbt-fusion compatible packages.
📚 Learning: 2025-10-05T16:25:01.719Z
Learnt from: haritamar
Repo: elementary-data/dbt-data-reliability PR: 825
File: macros/utils/cross_db_utils/incremental_strategy.sql:13-15
Timestamp: 2025-10-05T16:25:01.719Z
Learning: In macros/utils/cross_db_utils/incremental_strategy.sql, the get_default_incremental_strategy() macro and its adapter-specific variants are used exclusively for Elementary's internal dbt artifact tables, all of which have unique keys defined. Therefore, defaulting to "merge" strategy for Redshift (and other adapters) is safe in this context.

Applied to files:

  • macros/edr/tests/test_utils/clean_elementary_test_tables.sql
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
  • GitHub Check: test (latest_official, databricks_catalog) / test
  • GitHub Check: test (latest_official, clickhouse) / test
  • GitHub Check: test (fusion, bigquery) / test
  • GitHub Check: test (latest_official, snowflake) / test
  • GitHub Check: test (latest_pre, postgres) / test
  • GitHub Check: test (latest_official, bigquery) / test
  • GitHub Check: test (fusion, snowflake) / test
  • GitHub Check: test (latest_official, postgres) / test
  • GitHub Check: test (fusion, redshift) / test
  • GitHub Check: test (fusion, databricks_catalog) / test
  • GitHub Check: test (latest_official, redshift) / test
  • GitHub Check: test (latest_official, dremio) / test
  • GitHub Check: test (latest_official, trino) / test
  • GitHub Check: test (latest_official, athena) / test
🔇 Additional comments (1)
macros/edr/tests/test_utils/clean_elementary_test_tables.sql (1)

34-36: LGTM!

The new Redshift dialect-specific macro correctly delegates to the existing transactionless query generator, matching the established pattern used by BigQuery, Spark, Trino, and Dremio. This resolves the issue where Redshift refuses transactional delete statements by generating individual DROP TABLE IF EXISTS queries instead.


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

@FridayPush
Copy link

Can confirm that patching in this macro resolved our issue with 0.21.0. Thanks!

@GuyEshdat GuyEshdat merged commit 7b5fec6 into master Dec 17, 2025
32 checks passed
@GuyEshdat GuyEshdat deleted the redshift-remove-transcation-when-cleaning-test-temp-tables branch December 17, 2025 11:02
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.

4 participants