-
Notifications
You must be signed in to change notification settings - Fork 487
[sql_server] Implementent purification for SQL Server Source #32121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[sql_server] Implementent purification for SQL Server Source #32121
Conversation
bc87fc0 to
a4c2f00
Compare
a4c2f00 to
84c2d90
Compare
* implement purification for SQL Server sources, lists tables from upstream that have CDC enabled and their capture instances * returns an error for CREATE TABLE ... FROM SOURCE for SQL Server
84c2d90 to
04528d9
Compare
martykulma
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor notes/questions, nothing blocking 🚢🚢🚢
src/sql-server-util/src/inspect.rs
Outdated
| name, | ||
| capture_instance, | ||
| columns: columns.into(), | ||
| is_cdc_enabled: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is is_cdc_enabled implicitly true here because the table has a capture instance associated with it?
There's a ensure_table_cdc_enabled above, but it doesn't look like anything calls it. Wasn't sure if it's needed by an upcoming change or if it's vestigial because we can tell that cdc is enabled for a table via this query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
is_cdc_enabledimplicitly true here because the table has a capture instance associated with it?
It was! But this field also was unused so I got rid of it. Same with the ensure_table_cdc_enabled query, in a previous iteration it was used but no longer is.
* remove is_cdc_enabled field which was unused * fix Cargo.toml deps to get WASM build working * remove unused ensure_table_cdc_enabled function * refactor a bit of code
|
@martykulma TFTR! |
This PR implements "purification" in the Adapter for a SQL Server source. For folks unfamiliar, purification is a step during object creation where we query external systems for any necessary information, with the goal of making our persisted SQL "pure".
During purification of a SQL Server source we do the following:
Left as a TODO is implementing support for
CREATE TABLE ... FROM SQL SERVER. Nothing blocks our implementation of this feature although because it's not released yet I opted out of implementing it to keep the PR small.Motivation
Progress towards https://github.com/MaterializeInc/database-issues/issues/8762
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.