DanielLeens commented on PR #12003: URL: https://github.com/apache/seatunnel/pull/12003#issuecomment-5476938157
@SEZ9 thanks for the careful check before closing — I went and read #12001's actual merged diff (head `64456f89`) rather than just trusting the description. Here's what I found against your four points: **1. String-equality contract (row tableId vs. mapping key).** #12001 builds the map as `sinkTableMappings.put(sourceTable.toString(), sinkTable.getFullName())` and looks it up with `sinkTableMappings.getOrDefault(row.getTableId(), TablePath.DEFAULT.getFullName())` (`SinkFlowLifeCycle.java:205`, `:747-748` at the merge commit). `TablePath.toString()` delegates straight to `getFullName()`, and — as I verified in my own #12003 review — essentially every connector populates `row.setTableId()` via that same `getFullName()`/`toString()` form, matching how `MultiTableSinkWriter.write()` already routes rows by raw string identity. So the contract holds, but it holds the same way it does in this PR too: by codebase-wide convention, not by a single shared accessor that would fail to compile if a connector diverged. Not a gap specific to #12001 — same situation either fix would leave in place. **2. Silent fallback to `default.default.default`.** Confirmed still present in #12001 at the same two lines above — a lookup miss returns the default bucket with zero logging, identical to the pre-fix behavior and to this PR's own fix. I don't think this needs a separate flag on #12001's thread: it's not a regression either PR introduces, it's inherited from the original code, and it affects both fixes equally. Worth a small follow-up (a WARN log on fallback) regardless of which PR wins, but not something #12001 needs to fix that #12003 wouldn't also need to fix. **3. Test coverage — this is a real gap in #12001.** Its new test (`SinkFlowLifeCycleMetricsTest#schemaFirstTableIdUsesResolvedSinkTableMetrics`) only exercises the schema-first regression case, and it does so by mocking `MultiTableSink#getSinkTableMapping()` directly with Mockito rather than driving a real writer end-to-end. It does not cover: a db-first id (guarding the previously-working path from regressing), or an unmapped table id (guarding the default-fallback path). #12003's test, by contrast, goes through `TestSinkWriter`/`createMultiTableFlow` and explicitly asserts the default bucket stays at 0 alongside the positive case. So #12001's merged fix is correct, but its regression coverage is thinner than what's sitting unmerged in #12003. **4. Release changelog.** Doesn't apply to either PR — `release-note.md` was removed from the `dev` branch entirely (`a2a4de630db`, "[Improve] Remove release-note.md in dev branch"), so there's no manual changelog file left for either PR to update under the current release process. **Bottom line:** #12001's runtime fix is functionally equivalent and correct — same root cause, same string-identity key scheme, already merged. I don't see a reason to keep this PR open just to land it a second time. I'd go ahead and close #12003 with a pointer to #12001 as you suggested; separately, it'd be worth opening a small test-only follow-up (either on top of #12001 or as a fresh tiny PR) to add the db-first and unmapped-id/default-fallback assertions that #12003's test already had, since that gap is real and independent of which PR gets the credit. Thanks again for pushing on this rather than taking the dedup at face value. -- 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]
