Skip to content

oracledb_cdc: support snapshot filtering#4606

Merged
josephwoodward merged 7 commits into
mainfrom
jw/supportsnapshotfilters
Jul 17, 2026
Merged

oracledb_cdc: support snapshot filtering#4606
josephwoodward merged 7 commits into
mainfrom
jw/supportsnapshotfilters

Conversation

@josephwoodward

@josephwoodward josephwoodward commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

This change adds support for controlling the traditional greedy SELECT * FROM table_to_snapshot query, allowing the user to reduce the amount of data the snapshot will return, speeding up the snapshot process by excluding columns that aren't of interest or excluding certain groups of rows.

Given the following config:

image

We get the following filtered results:

image

Comment thread internal/impl/oracledb/replication/stream.go Outdated
Comment thread internal/impl/oracledb/replication/stream.go Outdated
Comment thread internal/impl/oracledb/input_oracledb_cdc.go
@claude

claude Bot commented Jul 17, 2026

Copy link
Copy Markdown

Commits

  1. oracledb_cdc: further work on filtering is vague — it does not describe what changed. Several commits (add snapshot filtering config, switch out query with filters, further work on filtering) read as incremental WIP steps of one feature and would be clearer squashed into self-contained logical units. Message format (system: message, lowercase imperative) is otherwise fine.

Review

The PR adds a snapshot.filters config to oracledb_cdc for per-table custom snapshot SELECT queries. Two build-breaking issues mean the feature does not work and the package does not compile as-is.

  1. Feature not wired + compile errorSnapshotFilters is parsed/validated but never passed to NewSnapshot; the call site (input_oracledb_cdc.go:555) still uses the old 8-argument form, so o.publisher is passed into the new filters map[string]string parameter (type mismatch, will not build). See inline comment.
  2. Missing dependencygithub.com/blastrain/vitess-sqlparser/sqlparser is imported in replication/stream.go but is absent from go.mod/go.sum; the module will not build. Run task deps and commit the result (CONTRIBUTING.md §6). See inline comment.
  3. Missing unit testsValidateSnapshotFilters / SnapshotFilterTablesExist are pure, branch-heavy validation functions (including SQL-injection guarding) with only a default-skipped integration test and no table-driven unit coverage of rejection cases. See inline comment.

@josephwoodward
josephwoodward force-pushed the jw/supportsnapshotfilters branch from c0fccf2 to 387ad46 Compare July 17, 2026 11:15
Comment thread internal/impl/oracledb/input_oracledb_cdc.go Outdated
@claude

claude Bot commented Jul 17, 2026

Copy link
Copy Markdown

Commits
LGTM — single commit oracledb_cdc: add snapshot filtering config follows the system: message format (lowercase, imperative) and cleanly describes one self-contained change.

Review
This PR adds per-table snapshot query filtering to the oracledb_cdc connector, aligning with CONTRIBUTING §5.5.5 (snapshot query filtering). The hand-rolled single-table SELECT validator is well-tested, and the snapshot query building (direct on the first batch, subquery-wrapped for cursor pagination) is sound. CDC-standard checks look good: no message-shape change, checkpoint_cache/checkpoint semantics untouched, and the cdctest deterministic gate still passes (canonical top-level fields unaffected).

One issue:

  1. The new snapshot/filters config field is not reflected in the generated docs page docs/modules/components/pages/inputs/oracledb_cdc.adoc, leaving it stale. CONTRIBUTING §6 requires running task docs and committing the regenerated page (CI fails on stale docs). See the inline comment.

@claude

claude Bot commented Jul 17, 2026

Copy link
Copy Markdown

Commits

  1. Commit 593a9e3 orcledb_cdc: update docs — typo in the system name (orcledb should be oracledb to match the component oracledb_cdc). Otherwise the format/mood are fine, and separating the docs change from the code change is correct.

Review
This PR adds per-table snapshot query filtering to oracledb_cdc via a new snapshot.filters map, a hand-rolled SQL validator, cursor-pagination wiring (subquery-wrap for subsequent batches), docs, and unit + integration tests. It implements CONTRIBUTING.md §5.5.5 (snapshot query filtering); the flat message shape, metadata keys, checkpoint behavior, and type mapping are all unchanged, so no §5 conformance regressions are introduced. Change is well-scoped and covered by tests.

LGTM

@josephwoodward
josephwoodward marked this pull request as ready for review July 17, 2026 11:32
Comment thread internal/impl/oracledb/replication/snapshot.go
@claude

claude Bot commented Jul 17, 2026

Copy link
Copy Markdown

Commits

  1. Commit orcledb_cdc: update docs (593a9e3) has a typo in the system name — orcledb_cdc should be oracledb_cdc. The commit-message system prefix must be a lowercase system name matching a known area; the misspelling does not match the component name. Granularity is otherwise good (docs split from the code change).

Review

The PR adds an optional per-table snapshot.filters config to oracledb_cdc, letting operators override the default snapshot SELECT per table, with a hand-rolled SQL validator that restricts filters to single-table SELECT statements. Config parsing, validation, existence checks, and both unit and integration tests are present and follow the project patterns.

  1. Custom snapshot query omitting the primary key breaks pagination. The first snapshot batch runs the custom query directly (ORDER BY "PK" is valid against the table even when the PK is not projected), but subsequent batches wrap it as SELECT * FROM (<customQuery>) t WHERE "PK" > :1 ORDER BY "PK", which fails with ORA-00904 when the PK column is not in the projection. Since the feature supports column projection (the integration test uses SELECT ID, NAME), this is a realistic path that fails mid-snapshot rather than at config time. Suggest validating that filters include the PK columns, or documenting it as a hard requirement (certification §1.2.3/§1.2.4). See inline comment on internal/impl/oracledb/replication/snapshot.go.

@claude

claude Bot commented Jul 17, 2026

Copy link
Copy Markdown

Commits

  1. Commit 593a9e3 — message orcledb_cdc: update docs misspells the system name (orcledboracledb), so the scope prefix does not match the known oracledb area.

Otherwise commit granularity (docs separated from code) and format are fine.

Review

This PR adds per-table snapshot query filtering to the oracledb_cdc connector via a new optional snapshot.filters object, with a hand-rolled single-table SELECT validator, cursor-pagination that wraps custom queries in a subquery, ORA-00904 detection for filters that omit primary-key columns, and both unit and integration test coverage. The RCL license headers on the new files are correct and the generated docs are included.

LGTM — no high-signal code issues found. The implementation is consistent with the existing snapshot code, the validator is well-tested, and the CDC message/config conventions (flat shape, canonical fields) are preserved.

@josephwoodward
josephwoodward force-pushed the jw/supportsnapshotfilters branch from 5d66add to 056d79c Compare July 17, 2026 12:08
Comment thread internal/impl/oracledb/replication/stream.go
@claude

claude Bot commented Jul 17, 2026

Copy link
Copy Markdown

Automated review by Claude.


Commits

  1. The commit orcledb_cdc: update docs misspells the system name — should be oracledb_cdc: to match the known area (it is oracledb_cdc in the other two commits). Minor, but the system prefix should be spelled consistently and match a real area.

Granularity and remaining message formats look good (docs are correctly separated from code changes into their own commit).

Review

Adds a snapshot.filters option to the oracledb_cdc connector (per-table snapshot query override), with a hand-rolled single-table SELECT validator, ORA-00904 diagnostics for filters that omit primary-key columns, and unit + integration tests. Implementation is solid and well-tested; no CDC-standard (§5) divergences found — the conformance gate (internal/plugins/cdctest) only checks canonical top-level fields and there is no canonical name for snapshot query filtering.

  1. Inconsistent case handling for filter keys (internal/impl/oracledb/replication/stream.go L139, snapshot.go L241): ValidateSnapshotFilters matches keys against the query table case-insensitively, but SnapshotFilterTablesExist and the processBatch lookup key on the always-uppercase UserTable.FullName(). A lowercase key (natural for case-insensitive Oracle identifiers) passes validation but then fails Connect() with a misleading "table not found in monitored tables" error. See inline comment.

@claude

claude Bot commented Jul 17, 2026

Copy link
Copy Markdown

Commits

  1. Commit orcledb_cdc: update docs misspells the system name — it should be oracledb_cdc: (missing the "a"). The system: prefix must match a known area, and this typo makes it not match. The other three commits are well-formed and the docs change is correctly isolated in its own commit.

Review

Reviewed the addition of per-table snapshot query filtering to the oracledb_cdc connector. The change is well-scoped and well-engineered: the hand-rolled SQL filter scanner is thoroughly unit-tested, the cursor-pagination wrapping ((customQuery) t) and the ORA-00904 detection are correctly placed for the second-batch failure mode described in the docs, filter-key casing is normalised consistently against catalog table names, and there is an integration test covering the feature end-to-end. The snapshot.filters config passes the internal/plugins/cdctest conformance gate (all canonical fields remain present) and nesting under a snapshot: object is consistent with the existing logminer: object.

LGTM

).Description("LogMiner configuration settings."),
).
// snapshot config
Field(service.NewObjectField(ociFieldSnapshot,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This introduces a nested snapshot: object (snapshot.filters) for snapshot configuration, whereas every other snapshot knob in this same connector is a flat, snapshot_-prefixed top-level field: stream_snapshot, max_parallel_snapshot_tables, snapshot_max_batch_size, snapshot_mode.

The CDC Connector Standard §5.3 asks new CDC config to "mirror the shape the fleet already establishes rather than introducing a new shape" (canonical snapshot fields are the flat snapshot_* names), and §3.1.5 asks connectors to feel consistent with the rest of the fleet. Having both a flat snapshot_max_batch_size and a nested snapshot.filters in one connector means two parallel ways to express snapshot config.

Consider a flat field (e.g. snapshot_filters) to stay consistent with the existing convention.

@claude

claude Bot commented Jul 17, 2026

Copy link
Copy Markdown

Commits

  1. orcledb_cdc: update docs — the system name is misspelled (orcledb → should be oracledb). The scope prefix should match the known system area exactly.
  2. oraledb_cdc: update log to highlight it used filtering — same class of typo in the system name (oraledboracledb).

Otherwise commit granularity is good (docs separated from code) and messages use lowercase imperative mood.

Review

Adds a per-table snapshot.filters override for the oracledb_cdc snapshot phase, with hand-rolled SQL validation (single-table SELECT only), case-insensitive key normalization, cursor-pagination via subquery wrapping, an ORA-00904 hint when a filter omits PK columns, and both unit and integration coverage. The implementation is clean, well-documented, and correctly handles the composite-PK/pagination edge cases it documents. No correctness bugs found.

  1. Config shape diverges from the fleet/connector convention: the new snapshot option is nested under a snapshot: object while all other snapshot knobs in this connector are flat snapshot_* fields. See inline comment (CDC Standard §5.3 / §3.1.5).

Comment thread internal/impl/oracledb/input_oracledb_cdc.go Outdated
@claude

claude Bot commented Jul 17, 2026

Copy link
Copy Markdown

Commits

Three commit messages have typos in the scope/message, which the enforced system: message format requires to match a known area:

  1. orcledb_cdc: update docs — system name misspelled (orcledboracledb).
  2. oraledb_cdc: update log to highlight it used filtering — system name misspelled (oraledboracledb).
  3. oracledb_cdc: flattern snapshot_filters — typo in message (flatternflatten).

These are minor; the granularity (docs kept separate from code) is otherwise fine.

Review

Adds a snapshot_filters config to oracledb_cdc allowing per-table custom snapshot SELECT queries, with a hand-rolled SQL validator restricting filters to single-table SELECTs. RCL license headers, distribution classification, and docs regeneration all look correct, and the validator/query logic is well-tested (unit + integration). One documentation-accuracy issue:

  1. The snapshot_filters field description (and mirrored .adoc) states the primary-key projection requirement only applies "if the table has a composite primary key". It applies to single-column primary keys too — a single-PK filter omitting the PK fails mid-snapshot with ORA-00904. See inline comment.

@claude

claude Bot commented Jul 17, 2026

Copy link
Copy Markdown

Commits

  1. orcledb_cdc: update docs — the system-name prefix is misspelled (orcledb_cdc). The enforced system: message format requires a system name matching a known area (oracledb_cdc).
  2. oraledb_cdc: update log to highlight it used filtering — same issue, the prefix is misspelled (oraledb_cdc, missing the c).
  3. Minor: oracledb_cdc: flattern snapshot_filters — "flattern" is a typo of "flatten" in the message body.

Review

Adds a snapshot_filters config to oracledb_cdc allowing per-table SQL SELECT overrides for the initial snapshot. The change stays conformant with the CDC fleet standard (flat message shape and metadata unchanged, snapshot query filtering per §5.5.5), the hand-rolled filter validator is well-structured and thoroughly unit-tested, error handling (ORA-00904 detection) is correct and wrapped once with %w, and the integration test math checks out.

LGTM

@josephwoodward
josephwoodward merged commit c9136a9 into main Jul 17, 2026
9 checks passed
@josephwoodward
josephwoodward deleted the jw/supportsnapshotfilters branch July 17, 2026 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants