Add support for `AlterTable::drop_columns` #106

amotl · 2025-11-27T22:39:05Z

About

This patch, adding the implementation for the drop_columns operation, satisfies SDK tester's schema_migrations_input_ddl.json.

coderabbitai · 2025-11-27T22:39:10Z

Warning

Rate limit exceeded

@amotl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 6 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 1ce0df3 and c1a3300.

📒 Files selected for processing (1)

src/cratedb_fivetran_destination/main.py (2 hunks)

Walkthrough

The PR extends the AlterTable operation to support column deletion. When drop_columns is requested, the system identifies columns present in the old table but absent in the new table, generates DROP COLUMN statements, executes them in a single ALTER TABLE command, and returns early.

Changes

Cohort / File(s)	Summary
Core functionality `src/cratedb_fivetran_destination/main.py`	Introduced `columns_deleted` collection and conditional logic to handle DROP COLUMN operations when `request.drop_columns` is true. After computing column diffs, identifies columns to drop, builds ALTER TABLE statements, executes them, and returns early before downstream logic.
Test data & configuration `tests/data/fivetran_migrations_ddl/configuration.json`, `tests/data/fivetran_migrations_ddl/schema_migrations_input_ddl.json`	Added test dataset with database writer configuration (CrateDB connection parameters) and DDL migration plan containing create_table, upsert, schema_migration operations (add_column, change_column_data_type, drop_column), and describe_table.
Unit tests `tests/test_adapter.py`	Added `test_api_alter_table_drop_column_nothing_changed` to exercise the drop_columns=True code path when no columns are actually dropped, validating successful response and log output.
Integration tests `tests/test_integration.py`	Added `test_integration_fivetran_migrations_ddl` parameterized test to validate end-to-end migrations DDL workflow against new test dataset, asserting expected output in stderr.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Main logic changes in main.py introduce a new conditional flow path for column deletion that requires careful verification against existing AlterTable logic
New control flow short-circuits subsequent primary key and recreate-alter logic; ensure interaction with PR #98 (primary key change handling) is correct
Multiple new test files and test functions require validation that they properly exercise the added functionality
Test data setup is straightforward but should be verified for representative coverage of drop_columns scenarios

Possibly related PRs

Implement AlterTableRecreateStatements for primary key column changes #98: Modifies AlterTable handling for primary key changes; this PR adds an early drop_columns path that precedes that logic, so sequencing and interaction between both flows should be verified.

Suggested reviewers

surister
seut

Poem

🐰 A hop, a skip, DROP COLUMN swift—
Fivetran's gifts keep coming adrift!
Old tables shrink, new schemas bloom,
Migrations dance through the digital room.
*wiggles nose* We've cleared the clutter with glee!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically summarizes the main change: adding support for AlterTable::drop_columns operation.
Description check	✅ Passed	The description relates to the changeset by referencing the SDK tester's schema_migrations_input_ddl.json that this PR is designed to support.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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.

❤️ Share

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

amotl · 2025-11-27T22:48:07Z

tests/data/fivetran_migrations_ddl/schema_migrations_input_ddl.json

+      "change_column_data_type": [
+        {
+          "table": "transaction",
+          "column": "amount",
+          "data_type": "STRING"


The target data type should actually be DOUBLE, but that currently fails.

Fivetran-Tester: schema_migrations_input_ddl.json fails with "Column 'amount' has data type DOUBLE but expected FLOAT" #105

Suggested change

"change_column_data_type": [

{

"table": "transaction",

"column": "amount",

"data_type": "STRING"

"change_column_data_type": [

{

"table": "transaction",

"column": "amount",

"data_type": "DOUBLE"

seut · 2025-11-28T06:35:37Z

src/cratedb_fivetran_destination/main.py

+                    for column in columns_deleted:
+                        connection.execute(
+                            sa.text(
+                                f'ALTER TABLE {table_info.fullname} DROP COLUMN "{column.name}"'


CrateDB supports dropping multiple columns at once, see https://cratedb.com/docs/crate/reference/en/latest/sql/statements/alter-table.html#drop-column.

Improved with a60831c, thanks.

seut · 2025-11-28T06:41:50Z

CHANGES.md

+- Added support for `AlterTable::drop_columns`, satisfying
+  SDK tester's `schema_migrations_input_ddl.json`


Isn't this more about adding support for changing data types as a user visible change?

I've compressed the statement.

This satisfies SDK tester's `schema_migrations_input_ddl.json`. Co-authored-by: Sebastian Utz <[email protected]>

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (2)

tests/test_adapter.py (1)
223-260: Good test coverage for the nothing-changed path.

The test correctly verifies that when drop_columns=True but no columns are actually removed, the operation succeeds with the appropriate log message.

Address the static analysis hint: prefix the unused err variable with an underscore:
     # Check log output.
-    out, err = capsys.readouterr()
+    out, _ = capsys.readouterr()
     assert format_log_message("AlterTable (drop columns): Nothing changed", newline=True) in out
tests/test_integration.py (1)
143-153: Integration test correctly validates migrations DDL workflow.

The test appropriately verifies that the Fivetran destination tester completes the migrations DDL scenario.

Address the static analysis hint: prefix the unused out variable with an underscore:
     # Read out stdout and stderr.
-    out, err = capfd.readouterr()
+    _, err = capfd.readouterr()
 
     assert "Describe Table: transaction" in err
Note: The static analysis warning about unused services parameter is a false positive—it's consumed by the pytest fixture mechanism.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7bc3da1 and 1ce0df3.

📒 Files selected for processing (6)

CHANGES.md (1 hunks)
src/cratedb_fivetran_destination/main.py (2 hunks)
tests/data/fivetran_migrations_ddl/configuration.json (1 hunks)
tests/data/fivetran_migrations_ddl/schema_migrations_input_ddl.json (1 hunks)
tests/test_adapter.py (1 hunks)
tests/test_integration.py (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

tests/test_adapter.py (3)

tests/conftest.py (1)

engine (6-17)

src/cratedb_fivetran_destination/main.py (2)

CrateDBDestinationImpl (31-351)

AlterTable (94-199)

src/cratedb_fivetran_destination/util.py (1)

format_log_message (14-19)

src/cratedb_fivetran_destination/main.py (3)

tests/conftest.py (1)

engine (6-17)

src/cratedb_fivetran_destination/model.py (2)

execute (196-198)

fullname (177-178)

src/cratedb_fivetran_destination/util.py (1)

log_message (10-11)

🪛 Ruff (0.14.6)

tests/test_adapter.py

258-258: Unpacked variable err is never used

Prefix it with an underscore or any other dummy variable pattern

(RUF059)

tests/test_integration.py

144-144: Unused function argument: services

(ARG001)

150-150: Unpacked variable out is never used

Prefix it with an underscore or any other dummy variable pattern

(RUF059)

⏰ 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). (2)

GitHub Check: oci
GitHub Check: Build for OS macos-15-intel

🔇 Additional comments (3)

CHANGES.md (1)

9-9: LGTM!

The changelog entry accurately describes the new feature.

tests/data/fivetran_migrations_ddl/configuration.json (1)

1-11: LGTM!

Test configuration is properly structured for the migrations DDL test scenario.

tests/data/fivetran_migrations_ddl/schema_migrations_input_ddl.json (1)

1-53: LGTM!

The DDL migration test data appropriately exercises the new drop_columns functionality along with other schema migration operations.

src/cratedb_fivetran_destination/main.py

amotl · 2025-11-28T11:45:26Z

tests/test_adapter.py

+    # Validate outcome.
+    assert response.success is True
+    assert response.warning.message == ""
+
+    # Check log output.
+    out, err = capsys.readouterr()
+    assert format_log_message("AlterTable (drop columns): Nothing changed", newline=True) in out


Maybe "nothing changed" should converge into a warning message instead? Let's track this on behalf of an "API polishing" ticket, please. /cc @coderabbitai

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash # Create a GitHub issue to track the API polishing suggestion gh issue create \ --title "API polishing: Convert 'nothing changed' messages to warnings" \ --body "## Context During review of PR #106, a suggestion was made to convert 'nothing changed' log messages into warning messages for better API consistency. Currently, when \`AlterTable\` operations result in no changes (e.g., when \`drop_columns=True\` but no columns are actually dropped), the system logs informational messages like: - \`AlterTable: Nothing changed\` - \`AlterTable (drop columns): Nothing changed\` ## Suggestion These 'nothing changed' scenarios should converge into warning messages instead of informational log messages, to better signal to users that the operation had no effect. ## References - PR: https://github.com/crate/cratedb-fivetran-destination/pull/106 - Comment: https://github.com/crate/cratedb-fivetran-destination/pull/106#discussion_r2571402896 - Requested by: @amotl" \ --assignee amotl

Length of output: 64

@amotl, I've created issue #108 to track this API polishing suggestion: #108

The issue captures the idea of converting "nothing changed" messages to warnings for better API consistency and has been assigned to you.

amotl temporarily deployed to pypi November 27, 2025 22:39 — with GitHub Actions Inactive

amotl force-pushed the migrations-ddl branch from 79d20ea to 6d6678d Compare November 27, 2025 22:42

amotl temporarily deployed to pypi November 27, 2025 22:42 — with GitHub Actions Inactive

amotl marked this pull request as ready for review November 27, 2025 22:42

amotl force-pushed the migrations-ddl branch from 6d6678d to 680b033 Compare November 27, 2025 22:44

amotl temporarily deployed to pypi November 27, 2025 22:44 — with GitHub Actions Inactive

amotl commented Nov 27, 2025

View reviewed changes

amotl requested review from seut and surister November 28, 2025 02:36

seut reviewed Nov 28, 2025

View reviewed changes

amotl force-pushed the migrations-ddl branch from 680b033 to a60831c Compare November 28, 2025 11:19

amotl temporarily deployed to pypi November 28, 2025 11:19 — with GitHub Actions Inactive

amotl requested a review from seut November 28, 2025 11:20

amotl force-pushed the make-it-work branch from df2894b to 175050e Compare November 28, 2025 11:22

seut approved these changes Nov 28, 2025

View reviewed changes

Base automatically changed from make-it-work to main November 28, 2025 11:39

Add support for AlterTable::drop_columns

1ce0df3

This satisfies SDK tester's `schema_migrations_input_ddl.json`. Co-authored-by: Sebastian Utz <[email protected]>

amotl force-pushed the migrations-ddl branch from a60831c to 1ce0df3 Compare November 28, 2025 11:40

amotl temporarily deployed to pypi November 28, 2025 11:40 — with GitHub Actions Inactive

coderabbitai bot reviewed Nov 28, 2025

View reviewed changes

src/cratedb_fivetran_destination/main.py Show resolved Hide resolved

amotl commented Nov 28, 2025

View reviewed changes

coderabbitai bot mentioned this pull request Nov 28, 2025

API polishing: Convert 'nothing changed' messages to warnings #108

Open

AlterTable::drop_columns: Implement suggestions by CodeRabbit

c1a3300

amotl temporarily deployed to pypi November 28, 2025 11:48 — with GitHub Actions Inactive

amotl merged commit 1947b59 into main Nov 28, 2025
11 checks passed

amotl deleted the migrations-ddl branch November 28, 2025 11:49

coderabbitai bot mentioned this pull request Nov 29, 2025

Schema migrations: Template file blueprints and minimal CrateDB implementation #107

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add support for `AlterTable::drop_columns` #106

Add support for `AlterTable::drop_columns` #106

Uh oh!

amotl commented Nov 27, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Nov 27, 2025 •

edited

Loading

Rate limit exceeded

Uh oh!

amotl Nov 27, 2025

Uh oh!

seut Nov 28, 2025

Uh oh!

amotl Nov 28, 2025

Uh oh!

seut Nov 28, 2025

Uh oh!

amotl Nov 28, 2025

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

amotl Nov 28, 2025

Uh oh!

coderabbitai bot Nov 28, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		- Added support for `AlterTable::drop_columns`, satisfying
		SDK tester's `schema_migrations_input_ddl.json`

Add support for AlterTable::drop_columns #106

Add support for AlterTable::drop_columns #106

Uh oh!

Conversation

amotl commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

About

Uh oh!

coderabbitai bot commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rate limit exceeded

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

amotl Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

seut Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

amotl Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

seut Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

amotl Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amotl Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add support for `AlterTable::drop_columns` #106

Add support for `AlterTable::drop_columns` #106

amotl commented Nov 27, 2025 •

edited

Loading

coderabbitai bot commented Nov 27, 2025 •

edited

Loading