Skip to content
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

source-oracle-batch: new connector #2387

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mdibaiee
Copy link
Member

@mdibaiee mdibaiee commented Feb 12, 2025

Description:

  • This is a copy-pasta from source-postgres-batch, with changes to queries, interfaces, etc. to make it an Oracle capture
  • The main deviation is the second commit, which is actually a problem postgres also has and needs to be fixed, I will file a separate PR for that

Workflow steps:

(How does one use this feature, and how has it changed)

Documentation links affected:

(list any documentation links that you created, or existing ones that you've identified as needing updates, along with a brief description)

Notes for reviewers:

(anything that might help someone review this PR)


This change is Reviewable

@mdibaiee mdibaiee force-pushed the mahdi/oracle-batch-connector branch 5 times, most recently from 7f33f9f to d32a10d Compare February 13, 2025 18:35
@mdibaiee mdibaiee force-pushed the mahdi/oracle-batch-connector branch from d32a10d to 38d4f84 Compare February 13, 2025 18:35
@mdibaiee mdibaiee marked this pull request as ready for review February 13, 2025 18:36
Id string `json:"ID"`
}

func TestCaptureCheckpointSCN(t *testing.T) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this behavior by setting scnCursor=false and running the test and seeing it fail, while with scnCursor=true it works 👍🏽

Copy link
Member

@willdonnelly willdonnelly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

The batch SQL captures are kind of a moving target at the moment, I'm in the process of doing some code health overhauls (see #2400 and #2406) and once those are complete I intend to add some new features like (optional) view discovery and the CDK-style full-refresh behavior with consistent keys and inferred deletions.

I think this implementation is good to merge as-is, just noting that there's a lot of stuff in the works that we'll need to apply to this connector as well.


RUN go install -v ./go/...
RUN go install -v ./source-boilerplate/...
RUN go install -v github.com/jackc/pgx/v5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably don't need PGX here?

Connect func(ctx context.Context, cfg *Config) (*sql.DB, error)
TranslateValue func(val any, databaseTypeName string) (any, error)
GenerateResource func(resourceName, schemaName, tableName, tableType string) (*Resource, error)
DefaultQueryTemplate string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've actually got a change out for review today (e931ede) which replaces this with a SelectQueryTemplate function.

The rationale here is that we've been talking about adding (probably optional) view discovery to the batch SQL captures, but of course views don't have any sort of XMIN / ROWSCN property for us to use so we'd probably want to make full-refresh and cursor-incremental capture behaviors work properly by default (that is, without forcing the user to input a query template).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants