[sql_server] add sql-server-cdc-old-syntax test suite #33265

ptravers · 2025-08-07T19:11:54Z

Motivation

This PR adds a known-desirable feature.

https://github.com/MaterializeInc/database-issues/issues/9534

Tips for reviewer

please note this was started on: #32241

Checklist

This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

ptravers · 2025-08-13T21:07:01Z

test/sql-server-cdc-old-syntax/types-money.td

+# SQL Server diverges from Postgres by returning numeric money.
+> SELECT pg_typeof(f1) FROM t1 LIMIT 1;
+"numeric"
+
+> SELECT * FROM t1;
+"922337203685477.58"
+"922337203685477.58"
+"-922337203685477.58"
+"-922337203685477.58"


we are deviating here from postgres. I'm assuming that this should be refactored to return text with dollar symbols?

ptravers · 2025-08-14T21:00:47Z

test/sql-server-cdc/mzcompose.py

@@ -234,7 +234,7 @@ def concurrent_updates(c: Composition) -> None:
        args=["--no-reset"],
        input=dedent(
            f"""
-            > SELECT COUNT(*) >= {update_rows+initial_rows}, MIN(id), MAX(id) >= {update_rows + update_id_offset - 1} FROM t1;
+            > SELECT COUNT(*) >= {update_rows + initial_rows}, MIN(id), MAX(id) >= {update_rows + update_id_offset - 1} FROM t1;


formatting changes from ruff when my editor wrote the file to fix some merge conflicts. is it okay to leave them in?

ptravers · 2025-08-14T21:01:20Z

test/sql-server-cdc/mzcompose.py

@@ -185,7 +185,7 @@ def workflow_snapshot_consistency(
    update_val_offset = 100000
    insert_delete = lambda i: dedent(
        f"""
-        INSERT INTO t1 VALUES (999999999,666666666), ({i+update_id_offset}, {i+update_val_offset});
+        INSERT INTO t1 VALUES (999999999,666666666), ({i + update_id_offset}, {i + update_val_offset});


formatting changes from ruff when my editor wrote the file to fix some merge conflicts. is it okay to leave them in?

I don't mind

def- · 2025-08-14T21:23:53Z

ci/test/pipeline.template.yml

+          # See: <https://github.com/microsoft/mssql-docker/issues/864>
+          queue: hetzner-x86-64-8cpu-16gb
+
+      - id: sql-server-cdc-old-syntax


I would move this to nightly once we have the same coverage in the sql-server-cdc (new syntax) tests

gotcha. makes sense.

def- · 2025-08-14T21:24:59Z

test/sql-server-cdc/mzcompose.py

@@ -185,7 +185,7 @@ def workflow_snapshot_consistency(
    update_val_offset = 100000
    insert_delete = lambda i: dedent(
        f"""
-        INSERT INTO t1 VALUES (999999999,666666666), ({i+update_id_offset}, {i+update_val_offset});
+        INSERT INTO t1 VALUES (999999999,666666666), ({i + update_id_offset}, {i + update_val_offset});


I don't mind

def- · 2025-08-14T21:25:17Z

test/sql-server-cdc-old-syntax/types-unsupported.td

+# the Business Source License, use of this software will be governed
+# by the Apache License, Version 2.0.
+
+# This file is not necessary for SQL Server but included for completeness.


I think removing it is better, runs faster

we should remove the empty mysql files with $nop in them then.

def- · 2025-08-14T21:26:31Z

ci/test/pipeline.template.yml

+        depends_on: build-x86_64
+        timeout_in_minutes: 30
+        inputs: [test/sql-server-cdc-old-syntax]
+        parallelism: 2


maybe also parallelism: 3

def- · 2025-08-14T21:27:23Z

ci/test/pipeline.template.yml

+        parallelism: 2
+        plugins:
+          - ./ci/plugins/mzcompose:
+              composition: sql-server-cdc


This is the same composition as the "SQL Server CDC tests"

fixed, result of a merge conflict

def- · 2025-08-14T21:27:34Z

ci/nightly/pipeline.template.yml

@@ -695,6 +695,16 @@ steps:
          queue: hetzner-aarch64-4cpu-8gb
        # the mzbuild postgres version will be used, which depends on the Dockerfile specification

+      - id: sql-server-cdc-old-syntax


Why is it both in nightly and test pipeline?

I wasn't sure where we would want it.

def- · 2025-08-15T07:54:15Z

As for the timeout in https://buildkite.com/materialize/test/builds/107879#0198ab1a-f76e-4362-83e8-3b8f834ae588

max-slot-wal-keep-size.td:55:1: error: query timed out

You can do $ set-sql-timeout duration=60s if it's expected to take longer.

def-

Other than the flake

ptravers requested a review from a team as a code owner August 7, 2025 19:11

ptravers force-pushed the 9534 branch from beaea87 to f0ed465 Compare August 7, 2025 19:12

ptravers changed the title ~~9534~~ [sql_server] add full sql server test suite Aug 7, 2025

ptravers force-pushed the 9534 branch 3 times, most recently from adbeb06 to a7f4bc4 Compare August 7, 2025 21:26

ptravers marked this pull request as draft August 7, 2025 21:32

ptravers force-pushed the 9534 branch 4 times, most recently from 10b169c to 381fe99 Compare August 13, 2025 13:12

ptravers mentioned this pull request Aug 13, 2025

[sql_server] remove enabling sql server in test as defaults to enabled #33315

Merged

5 tasks

ptravers force-pushed the 9534 branch from f13baed to 3fd48a9 Compare August 13, 2025 14:51

ptravers changed the title ~~[sql_server] add full sql server test suite~~ [sql_server] add sql-server-cdc-old-syntax test suite Aug 13, 2025

ptravers marked this pull request as ready for review August 13, 2025 15:05

ptravers force-pushed the 9534 branch 3 times, most recently from d8f3117 to aceab21 Compare August 13, 2025 21:05

ptravers commented Aug 13, 2025

View reviewed changes

ptravers force-pushed the 9534 branch 5 times, most recently from 7c349dc to 41683db Compare August 14, 2025 20:00

ptravers requested review from petrosagg, def- and martykulma August 14, 2025 20:01

add sql-server-cdc-old-syntax tests

6f6ffb3

ptravers force-pushed the 9534 branch from 41683db to 6f6ffb3 Compare August 14, 2025 20:57

ptravers commented Aug 14, 2025

View reviewed changes

ptravers force-pushed the 9534 branch 2 times, most recently from 2b53b0b to 79a6f9c Compare August 14, 2025 21:15

def- requested changes Aug 14, 2025

View reviewed changes

ptravers force-pushed the 9534 branch from 79a6f9c to 2c3117c Compare August 14, 2025 21:54

fix pr feedback

6b17a19

ptravers force-pushed the 9534 branch from 2c3117c to 6b17a19 Compare August 14, 2025 22:01

remove from nightly

c51c0c7

ptravers force-pushed the 9534 branch 6 times, most recently from d8ce061 to 1809978 Compare August 14, 2025 23:48

ptravers requested a review from def- August 14, 2025 23:49

ptravers force-pushed the 9534 branch from 1809978 to 10facb5 Compare August 14, 2025 23:50

add new line to test files

05e2000

ptravers force-pushed the 9534 branch 2 times, most recently from e4f0718 to a52b205 Compare August 15, 2025 00:21

def- approved these changes Aug 15, 2025

View reviewed changes

ptravers added 2 commits August 15, 2025 10:09

ci: increase parallelism

fae5004

fix flake by increasing timeout for long running query

fcfd51a

ptravers force-pushed the 9534 branch from a52b205 to fcfd51a Compare August 15, 2025 14:09

ptravers enabled auto-merge August 15, 2025 14:10

ptravers disabled auto-merge August 15, 2025 14:10

ptravers merged commit 9130ef6 into MaterializeInc:main Aug 15, 2025
134 checks passed

ptravers deleted the 9534 branch August 15, 2025 14:33

[sql_server] add sql-server-cdc-old-syntax test suite #33265

[sql_server] add sql-server-cdc-old-syntax test suite #33265

Uh oh!

Conversation

ptravers commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Tips for reviewer

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

def- commented Aug 15, 2025

Uh oh!

def- left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ptravers commented Aug 7, 2025 •

edited

Loading