DanielLeens commented on PR #11399:
URL: https://github.com/apache/seatunnel/pull/11399#issuecomment-5846255668
Thanks for reviewing this independently, and for taking it further than I
did. You reached the same conclusion I flagged yesterday on this exact head
(`97d57923f66a`) — that populating `database.include.list` in
`PostgresSourceConfigFactory` doesn't stay scoped to database-level discovery,
it also feeds the shared `dataCollectionFilter()` that other Postgres CDC code
paths consume with catalog-less `TableId`s — but you traced it more completely
than I did, and I want to confirm I checked your additions rather than just
taking them at face value.
I verified `PostgresSnapshotSplitReadTask.resolveTable` (line 174) and
`PostgresSourceFetchTaskContext.getTableId` (line 403) both construct `new
TableId(null, schema, table)`, and that both event dispatchers in
`PostgresSourceFetchTaskContext` (lines 273, 290) are wired with
`connectorConfig.getTableFilters().dataCollectionFilter()` — the same
predicate. I hadn't cited those call sites myself; good catch, and it makes the
blast radius clearly bigger than "snapshot schema reads only." The
`Db2SourceConfigFactory` reference is exactly on point too — I confirmed that
comment exists verbatim in this repo (lines 63-65): this connector already
learned this lesson once and deliberately does not set `database.include.list`
for the same reason.
Your Issue 3 is the most useful new piece here. I checked the
`tableFilters()` helper in the current `TableDiscoveryUtilsTest` and confirmed
it's built from `databaseList("testdb")`, and that `#12435`'s two dedup tests
feed rows with `TABLE_CATALOG="highgo"`. With `database.include.list=testdb` in
effect, `dataCollectionFilter()` will reject every `highgo`-catalog row on the
catalog check alone — so `shouldDeduplicateRepeatedCatalogRows` drops from 1
table to 0, and `shouldKeepFirstOccurrenceWhenDuplicatesInterleave` drops from
3 entries to the 2 you listed. That's a precise, reproducible regression
against already-merged behavior, not a hypothetical, and it strengthens the
case that this needs fixing before merge rather than as a fast-follow.
Given the convergence, I think the fix direction is settled: don't set
`database.include.list` at all. Build the database-level predicate from
`postgresSourceConfig.getDatabaseList()` directly (an explicit
`Set<String>`/`Predicate<String>` passed into
`TableDiscoveryUtils.listTables`), so `dataCollectionFilter()` stays exactly as
permissive as it is on `dev` today. That single change should close your Issue
1, my 09-25 finding, and Issue 3 together, since the catalog-based rejection
disappears entirely once the shared filter is never touched. Your Issue 2
test-coverage ask (build a real `RelationalTableFilters` via the existing
`tableFilters()` helper and assert a catalog-less `TableId` is still included)
is the right regression guard for it, and worth adding alongside the fix in the
same commit. Issue 4 (Javadoc on the new two-stage filtering contract, and
dropping the exclusion log to `DEBUG`) is a reasonable small addition while
that method is already being touched. Issues 5-7
(the unclosed `MockJdbcConnection`, the brittle exact-SQL assertions, and the
style nits) have been open since your 08-23 review — I'd still treat them as
non-blocking on their own, but since a new commit is coming anyway to address
Issues 1-3, bundling them in would close out this review's long tail in one
pass rather than leaving them as a fast-follow.
To be clear on status: this remains not-ready-to-merge on my side until the
`database.include.list` side effect is removed and the `#12435` regression is
confirmed fixed (your Issue 3 gives an exact, checkable way to confirm that —
those two tests should go back to their current `dev` expectations). Once a new
commit lands, I'll do a full fresh re-review of the whole diff rather than an
incremental one, consistent with how I've handled the prior rounds on this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
