source-mysql-batch: Paying down some technical debt #2400
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
willdonnelly
added
the
change:unplanned
willdonnelly
force-pushed
the
wgd/batch-mysql-default-template-20250212
branch
from
01ffa90 to
d78da40
Compare
This modifies source-mysql-batch to behave similarly to how source-postgres-batch handles query templates and generated resource specs -- instead of hard-coding a complicated and ugly query template by default, we instead just put the table name and schema in the resource spec and say that if the template field is blank we apply a default query template which knows how to plug in the table name and schema. This is pretty much just a tech debt reduction thing, there are good reasons we did it this way for Postgres and it's good to have all the batch SQL captures be roughly similar in how they behave.
Getting ready to start implementing a bunch of tests.
It's well past time that we ought to have proper test coverage of datatype support in the MySQL Batch capture connector. This commit adds tests which exercise almost all MySQL column types.
Adds more test scenarios, including: - A full-refresh table with no cursor. - A polling interval which yields no results. - Modifications and deletions as well as inserts. - Null values in the cursor column. - A two-column compound cursor. - An explicit query template and no table/schema names, like what all preexisting captures in production have.
willdonnelly
force-pushed
the
wgd/batch-mysql-default-template-20250212
branch
from
c797247 to
bd0f76d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
This PR takes the
source-mysql-batchtest suite from "hilariously poorly tested" to "probably a bit over-tested" with about 700 lines of new test code covering all the important MySQL column types as well as various edge cases of key/cursor handling.This PR also adds
tableandschemaproperties to the resource config struct and implements the same logic thatsource-postgres-batchhas had for some time where if a query template is specified in the resource config we use it, but otherwise the connector knows what default query to use and plugs in the table/schema name from the resource into that hard-coded default, and when discovering new tables we always leave the template override blank.I intend to do both of these things for the other batch SQL capture connectors in the near future, because I would really like to be able to make changes to them all with more confidence.
Workflow steps:
Nothing should change for existing users. New users will no longer have a big ugly template to look at if they inspect the discovered resource bindings, but they still have the option of writing their own if they really want to.
Notes:
If you look closely at the datatype test snapshots you might notice that our serialization of some column types is...less than optimal. We might want to consider improving that soon, but right now my goal was just to document via snapshots the current connector behavior so we can at least tell if a future change impacts anything.
This change is