feat: Add systemdb support for MS SQL Server #6167

JulesHuisman · 2022-06-12T15:11:13Z

Relates to issue #3238.

The main improvement is limiting all String types in the alembic migrations. This allows the possibility to use both MSSQL and MYSQL as a Meltano backend database.

Both databases were manually tested. It might be valuable to add automatic tests to the Github actions.

To use MYSQL run: pip install mysqlclient
To use MSSQL run: pip install pyodbc

netlify · 2022-06-12T15:11:15Z

👷 Deploy request for meltano pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	`7fa5e19`

pandemicsyn · 2022-06-13T22:17:51Z

Hi @JulesHuisman awesome to see you pick this back up, personally very excited to get some support for other DB's into Meltano 🥳

@aaronsteers this is a continuation of this thread: https://gitlab.com/meltano/meltano/-/issues/3315#note_908238671, I think what @JulesHuisman has here plus some docs covering how to use "unofficial backends like MSSQL and MySQL" are a great start and easy way to test the water.

We could follow it up and setup an actions workflow to test migrations against MySQL/MSSQL when a PR with new migrations is created. @aaronsteers In the original thread you'd expressed some concerns about being able to test against MSSQL, looks like the MSSQL on linux docker container is pretty popular and well-supported (https://hub.docker.com/_/microsoft-mssql-server), but just having a GH actions gate for migrations would go along way towards ensuring that future migrations don't having breaking changes for MySQL or MSSQL.

aaronsteers · 2022-06-14T00:35:11Z

@JulesHuisman and @pandemicsyn - I would be happy to add CI tests for DBs. I am also okay to merge this and add those tests in a follow-on issue, depending on @JulesHuisman's availability and/or others who can help ship this in CI.

A quick google search shows tests may be fairly pretty easy in GitHub Actions - https://datastation.multiprocess.io/blog/2021-12-16-sqlserver-in-github-actions.html

src/meltano/migrations/versions/13e8639c6d2b_add_state_edit_to_job_state_enum.py

aaronsteers · 2022-06-14T00:39:33Z

@pandemicsyn and @edgarrmondragon - I'm removing myself from reviewer role but please re-add me if you would like me to rereview. I'll defer to you both - when you feel this is ready to merge, I'm all for it. 👍

And - again - since risk here is super low (adding str max when where they were otherwise omitted), I'm okay to merge without additional CI checks added.

The one thing I'd ask to watch out for is that I'd rather a stringlen max be overly permissive than to run a risk of truncating or failing on use cases that might introduce longer strings. (Most modern data platforms don't have any benefit to shorter lengths anyway, since they only use the num bytes that each row requires.)

edgarrmondragon · 2022-06-14T00:48:10Z

edgarrmondragon · 2022-06-14T00:58:20Z

@pandemicsyn we used to have a sort of test matrix running tests on both SQLite and PostgreSQL:

meltano/tests/conftest.py

Lines 21 to 38 in abb8315

    
           PYTEST_BACKEND = os.getenv("PYTEST_BACKEND", "sqlite") 
        
           pytest_plugins = [ 
        
               "fixtures.db", 
        
               "fixtures.fs", 
        
               "fixtures.core", 
        
               "fixtures.api", 
        
               "fixtures.cli", 
        
           ] 
        
           if PYTEST_BACKEND == "sqlite": 
        
               pytest_plugins.append("fixtures.db.sqlite") 
        
           elif PYTEST_BACKEND == "postgresql": 
        
               pytest_plugins.append("fixtures.db.postgresql") 
        
           else: 
        
               raise Exception(f"Unsuported backend: {PYTEST_BACKEND}.") 
        
           BACKEND = ["sqlite", "postgresql"]

meltano/.gitlab/ci/test.gitlab-ci.yml

Lines 88 to 94 in 1498bf5

    
           .pytest_sqlite: 
        
             extends: .pytest 
        
             variables: 
        
               PYTEST_BACKEND: sqlite 
        
               # `target-sqlite` configuration 
        
               SQLITE_DATABASE: pytest_warehouse

meltano/.gitlab/ci/test.gitlab-ci.yml

Lines 110 to 113 in 1498bf5

    
           pytest_postgres: 
        
             extends: .pytest_postgres 
        
             variables: 
        
               PYTEST_MARKERS: not concurrent

I suggest we migrate those (either before or after this PR merges) and include both MySQL and MSSQL in the mix.

JulesHuisman · 2022-06-14T08:16:16Z

Thanks for taking a look guys,

In some places it might be possible to make the max string length longer. I chose 128 because in some places MYSQL would throw errors when it was longer (450).

I can also take a look at testing the migrations. The MSSQL docker container works quite well, I also used it when developing this PR.

JulesHuisman · 2022-06-18T08:43:02Z

@edgarrmondragon @pandemicsyn Hi guys, what needs to be fixed or adjusted to get this PR through?

And do we want the tests included in this PR, or can that be included in a separate PR?

Thanks!

edgarrmondragon · 2022-06-21T21:15:22Z

@JulesHuisman can you merge main into this branch? We just added Postgres to the test matrix there.

…b-support

codecov · 2022-06-22T08:06:48Z

Codecov Report

Merging #6167 (7fa5e19) into main (ef4428f) will increase coverage by 0.02%.
The diff coverage is 86.11%.

@@            Coverage Diff             @@
##             main    #6167      +/-   ##
==========================================
+ Coverage   96.52%   96.55%   +0.02%     
==========================================
  Files          95       96       +1     
  Lines        8349     8500     +151     
  Branches      398      405       +7     
==========================================
+ Hits         8059     8207     +148     
  Misses        227      227              
- Partials       63       66       +3

Impacted Files	Coverage Δ
tests/conftest.py	`91.48% <66.66%> (+0.28%)`	⬆️
tests/fixtures/db/mssql.py	`87.50% <87.50%> (ø)`
tests/fixtures/db/postgresql.py	`100.00% <100.00%> (ø)`
tests/meltano/cli/test_config.py	`89.55% <0.00%> (-3.05%)`	⬇️
tests/meltano/core/test_settings_store.py	`99.14% <0.00%> (-0.07%)`	⬇️
tests/meltano/core/runner/test_runner.py	`99.10% <0.00%> (-0.02%)`	⬇️
tests/asserts.py	`100.00% <0.00%> (ø)`
tests/fixtures/api.py	`100.00% <0.00%> (ø)`
tests/fixtures/db/sqlite.py	`100.00% <0.00%> (ø)`
tests/meltano/cli/test_ui.py	`100.00% <0.00%> (ø)`
... and 84 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

JulesHuisman · 2022-06-22T08:40:27Z

Ah alright, so it is running into issues with the string length. I will look into it!

tests/fixtures/db/mssql.py

…ulesHuisman/meltano into 3238-broader-system-db-support

edgarrmondragon · 2022-06-22T21:51:34Z

@JulesHuisman an options seems to be off with the mssql service, and it's causing all workflows to fail.

We moved away from the services approach in favor of using docker run explicitly in a step (see #6268). What do you think of following a similar approach for mysql and mssql?

JulesHuisman · 2022-06-22T21:54:53Z

@JulesHuisman an options seems to be off with the mssql service, and it's causing all workflows to fail.

We moved away from the services approach in favor of using docker run explicitly in a step (see #6268). What do you think of following a similar approach for mysql and mssql?

Ah yes, looks good! I will merge the new main branch and switch over mssql and mysql. (I think my health check needs a health check itself 😃)

joaopamaral · 2022-07-27T20:54:24Z

Hi @JulesHuisman! First of all, thanks for this awesome work!

I just had to include a small change to make it work in my environment in the src/meltano/migrations/versions/5b43800443d1_rename_job_to_job_run_and_job_id_to_job_.py migration file to include the existing_type required by alembic.

The new file looks like this after the changes:

import sqlalchemy as sa
from alembic import op

from meltano.migrations.utils.dialect_typing import (
    get_dialect_name,
    max_string_length_for_dialect,
)

# revision identifiers, used by Alembic.
revision = "5b43800443d1"
down_revision = "13e8639c6d2b"
branch_labels = None
depends_on = None


def upgrade():
    dialect_name = get_dialect_name(op)
    max_string_length = max_string_length_for_dialect(dialect_name)

    op.alter_column("job", "job_id", 
        new_column_name="job_name",
        existing_type=sa.types.String(max_string_length)
    )
    op.rename_table("job", "runs")


def downgrade():
    dialect_name = get_dialect_name(op)
    max_string_length = max_string_length_for_dialect(dialect_name)
    
    op.rename_table("runs", "job")
    op.alter_column("job", "job_name", 
        new_column_name="job_id",
        existing_type=sa.types.String(max_string_length)
    )

After this change, I managed to make the db migration work with a MySQL instance.

edgarrmondragon · 2022-07-27T21:29:46Z

@joaopamaral thanks for sharing! I've put your changes in diff format in case @JulesHuisman or someone else wants to try them out:

 import sqlalchemy as sa
 from alembic import op
 
+from meltano.migrations.utils.dialect_typing import (
+    get_dialect_name,
+    max_string_length_for_dialect,
+)
+
 # revision identifiers, used by Alembic.
 revision = "5b43800443d1"
 down_revision = "13e8639c6d2b"
 
 
 def upgrade():
-    op.alter_column("job", "job_id", new_column_name="job_name")
+    dialect_name = get_dialect_name(op)
+    max_string_length = max_string_length_for_dialect(dialect_name)
+
+    op.alter_column("job", "job_id", 
+        new_column_name="job_name",
+        existing_type=sa.types.String(max_string_length)
+    )
     op.rename_table("job", "runs")
 
 
 def downgrade():
+    dialect_name = get_dialect_name(op)
+    max_string_length = max_string_length_for_dialect(dialect_name)
+    
     op.rename_table("runs", "job")
-    op.alter_column("job", "job_name", new_column_name="job_id")
+    op.alter_column("job", "job_name", 
+        new_column_name="job_id",
+        existing_type=sa.types.String(max_string_length)
+    )

edgarrmondragon · 2022-07-28T20:22:55Z

Heads up @JulesHuisman: I'm gonna merge main into this branch to try to resolve the conflicts.

edgarrmondragon · 2022-07-28T23:08:19Z

@pandemicsyn @aaronsteers this is ready for review for MSSQL support!

pandemicsyn

Awesome! 🥳

JulesHuisman · 2022-07-29T06:47:09Z

Thanks @edgarrmondragon, I like the idea of moving the Mysql code to another branch. That created a really weird bug in testing which I could not figure out.

edgarrmondragon · 2022-08-02T21:33:52Z

Thanks @edgarrmondragon, I like the idea of moving the Mysql code to another branch. That created a really weird bug in testing which I could not figure out.

@JulesHuisman I started a draft PR #6528, but I (or someone else) need to take some to debug those failing state tests.

aaronsteers

Awesome work here! Very excited to get this in!

tayloramurphy

Minor comments but I'm approving assuming those can get fixed so it's not a blocker

docs/src/_concepts/project.md

docs/src/_guide/advanced-topics.md

JulesHuisman · 2022-08-05T20:07:54Z

Thanks for taking this over the finish line @edgarrmondragon!

JulesHuisman added 2 commits June 12, 2022 15:00

Add a max string length to migrations

646b74c

Added a string limit to JSONEncodedDict

58355e5

JulesHuisman requested review from pandemicsyn and edgarrmondragon as code owners June 12, 2022 15:11

pandemicsyn requested a review from aaronsteers June 13, 2022 22:04

aaronsteers reviewed Jun 14, 2022

View reviewed changes

src/meltano/migrations/versions/13e8639c6d2b_add_state_edit_to_job_state_enum.py Show resolved Hide resolved

edgarrmondragon mentioned this pull request Jun 14, 2022

Run tests against multiple databases #6198

Closed

Merge remote-tracking branch 'origin/main' into 3238-broader-system-d…

97f5e4d

…b-support

JulesHuisman and others added 6 commits June 22, 2022 20:46

Fixed typing to work with MSSQL backends

14bf82b

Added two more dynamic datetime types

fe63148

Altered migrations to work with MYSQL

42c4691

Added MSSQL testing to Github workflow

4778d48

Added MySQL to the Github workflow

99e4d49

Merge branch 'main' into 3238-broader-system-db-support

b11099b

JulesHuisman commented Jun 22, 2022

View reviewed changes

tests/fixtures/db/mssql.py Outdated Show resolved Hide resolved

JulesHuisman added 2 commits June 22, 2022 21:44

Hard coded string lengths in migrations

def2f98

Merge branch '3238-broader-system-db-support' of https://github.com/J…

1fe4730

…ulesHuisman/meltano into 3238-broader-system-db-support

tayloramurphy added the Community-Contributed PR label Jul 27, 2022

Merge branch 'main' into 3238-broader-system-db-support

6d9a826

edgarrmondragon changed the title ~~feat: add systemdb support for MySQL, MS SQL Server, and others~~ feat: Add systemdb support for MS SQL Server Jul 28, 2022

edgarrmondragon approved these changes Jul 28, 2022

View reviewed changes

edgarrmondragon requested a review from aaronsteers July 28, 2022 23:07

Remove MySQL code

9edbb6f

edgarrmondragon force-pushed the 3238-broader-system-db-support branch from bf09f4d to 9edbb6f Compare July 28, 2022 23:28

This was referenced Jul 28, 2022

feat: Add systemdb support for MySQL #6528

Closed

feature: Support for MySQL as a backend database #6529

Open

pandemicsyn approved these changes Jul 29, 2022

View reviewed changes

WillDaSilva approved these changes Jul 29, 2022

View reviewed changes

edgarrmondragon added 2 commits July 29, 2022 12:36

Merge branch 'main' into 3238-broader-system-db-support

1a1370c

Merge branch 'main' into 3238-broader-system-db-support

8afea8b

Add guide for installing extra components

f9da1b2

JulesHuisman requested review from afolson, tayloramurphy and a team as code owners August 5, 2022 18:17

aaronsteers approved these changes Aug 5, 2022

View reviewed changes

tayloramurphy approved these changes Aug 5, 2022

View reviewed changes

docs/src/_concepts/project.md Outdated Show resolved Hide resolved

docs/src/_guide/advanced-topics.md Outdated Show resolved Hide resolved

Clarify extras are Python/pip extras

7fa5e19

edgarrmondragon merged commit 9dc9075 into meltano:main Aug 5, 2022

pandemicsyn mentioned this pull request Sep 16, 2022

chore: Support "state backends" beginning with systemdb #6742

Merged

edgarrmondragon mentioned this pull request Mar 8, 2024

feature: System database support for AWS S3 #8435

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add systemdb support for MS SQL Server #6167

feat: Add systemdb support for MS SQL Server #6167

JulesHuisman commented Jun 12, 2022

netlify bot commented Jun 12, 2022 •

edited

Loading

pandemicsyn commented Jun 13, 2022

aaronsteers commented Jun 14, 2022

aaronsteers commented Jun 14, 2022 •

edited

Loading

edgarrmondragon commented Jun 14, 2022 •

edited

Loading

Jobs

Plugin Settings

`embed_tokens`

Subscriptions

User

Role

Role Permissions

OAuth

edgarrmondragon commented Jun 14, 2022 •

edited

Loading

JulesHuisman commented Jun 14, 2022

JulesHuisman commented Jun 18, 2022

edgarrmondragon commented Jun 21, 2022 •

edited

Loading

codecov bot commented Jun 22, 2022 •

edited

Loading

JulesHuisman commented Jun 22, 2022

edgarrmondragon commented Jun 22, 2022

JulesHuisman commented Jun 22, 2022 •

edited

Loading

joaopamaral commented Jul 27, 2022

edgarrmondragon commented Jul 27, 2022

edgarrmondragon commented Jul 28, 2022

edgarrmondragon commented Jul 28, 2022

pandemicsyn left a comment

JulesHuisman commented Jul 29, 2022

edgarrmondragon commented Aug 2, 2022

aaronsteers left a comment

tayloramurphy left a comment

JulesHuisman commented Aug 5, 2022

feat: Add systemdb support for MS SQL Server #6167

feat: Add systemdb support for MS SQL Server #6167

Conversation

JulesHuisman commented Jun 12, 2022

netlify bot commented Jun 12, 2022 • edited Loading

👷 Deploy request for meltano pending review.

pandemicsyn commented Jun 13, 2022

aaronsteers commented Jun 14, 2022

aaronsteers commented Jun 14, 2022 • edited Loading

edgarrmondragon commented Jun 14, 2022 • edited Loading

Jobs

Plugin Settings

embed_tokens

Subscriptions

User

Role

Role Permissions

OAuth

edgarrmondragon commented Jun 14, 2022 • edited Loading

JulesHuisman commented Jun 14, 2022

JulesHuisman commented Jun 18, 2022

edgarrmondragon commented Jun 21, 2022 • edited Loading

codecov bot commented Jun 22, 2022 • edited Loading

Codecov Report

JulesHuisman commented Jun 22, 2022

edgarrmondragon commented Jun 22, 2022

JulesHuisman commented Jun 22, 2022 • edited Loading

joaopamaral commented Jul 27, 2022

edgarrmondragon commented Jul 27, 2022

edgarrmondragon commented Jul 28, 2022

edgarrmondragon commented Jul 28, 2022

pandemicsyn left a comment

Choose a reason for hiding this comment

JulesHuisman commented Jul 29, 2022

edgarrmondragon commented Aug 2, 2022

aaronsteers left a comment

Choose a reason for hiding this comment

tayloramurphy left a comment

Choose a reason for hiding this comment

JulesHuisman commented Aug 5, 2022

netlify bot commented Jun 12, 2022 •

edited

Loading

aaronsteers commented Jun 14, 2022 •

edited

Loading

edgarrmondragon commented Jun 14, 2022 •

edited

Loading

`embed_tokens`

edgarrmondragon commented Jun 14, 2022 •

edited

Loading

edgarrmondragon commented Jun 21, 2022 •

edited

Loading

codecov bot commented Jun 22, 2022 •

edited

Loading

JulesHuisman commented Jun 22, 2022 •

edited

Loading