Repository navigation
[fix](paimon) Stop clearing the Paimon table cache on every table lookup - #68860
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#67023, apache#67395, apache#67996 Problem Summary: master apache#67395 forward-ported branch-4.1 apache#67023, which keeps the Paimon write cache coherent. In branch-4.x the Doris table cache sits above Paimon's SDK CachingCatalog, and on a Doris cache miss apache#67023 invalidates the SDK entry before reloading, so the reload cannot return a stale SDK table. apache#67395 put that invalidation into `CatalogBackedPaimonCatalogOps.getTable`. On master, however, apache#67996 had already replaced the SDK CachingCatalog with the Doris-owned `PaimonMetaCacheCatalog` (the SDK wrapper is always disabled), so the catalog this method wraps is the Doris cache itself. `getTable` runs on every `getTableHandle`, several times per statement, and each call invalidated the table's cached metadata (the table, its latest snapshot, the schema-at memo and the partition view) and reloaded it from the remote catalog. As a result the Paimon metadata cache never hit. On a local filesystem catalog, 7 SELECT statements made 86 table-cache requests with 0 hits and 38 invalidations, and every statement reloaded the table and its latest snapshot; on HMS, REST or object-store catalogs each reload is a remote call. A catalog with caching enabled also stopped serving its cached snapshot. apache#67395 changed `test_paimon_table_meta_cache` to expect rows written by Spark without a REFRESH and attributed that to Paimon 1.4, but branch-4.2, on the same Paimon 1.4.2, still expects the cached rows. This removes the invalidation. REFRESH TABLE / CATALOG, Doris writes (`refreshTableAfterExternalMutation`) and DDL invalidate through `CatalogMetaCache`, which clears the `PaimonMetaCacheCatalog` entries, and no other layer caches the table. The regression expectation is restored, and the unit test that pinned the invalidation is replaced by one that pins cache hits until the table is invalidated. ### Release note None ### Check List (For Author) - Test: Unit Test, Manual test; Regression test (not run locally) - New `PaimonMetaCacheCatalogTest#connectorLookupsHitTheTableCacheUntilTheTableIsInvalidated` fails when the invalidation is put back; all 675 tests of `fe-connector-paimon` pass. - Local FE + BE with a filesystem Paimon catalog: after another catalog writes the table, the cached catalog reads the cached rows until REFRESH TABLE, and reads its own writes at once; five repeated SELECTs add 20 table-cache requests, all hits (before: 0 hits). - `test_paimon_table_meta_cache` needs the Paimon docker fixture and was not run locally. - Behavior changed: Yes. A Paimon catalog with metadata caching enabled serves repeated queries from its cache again; data committed by other engines becomes visible after REFRESH or the cache TTL, as in branch-4.x and on master before apache#67395. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
|
run buildall |
|
/review |
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Complete static review of PR #68860 at e864467. Two review rounds converged with no new valuable findings in round 2. There were no existing inline threads or additional user focus points.
Critical checkpoints:
- Goal and proof: Removing the per-lookup invalidation permits repeated Paimon table lookups to hit the Doris cache, as the new unit test checks. The changed external-INSERT regression asserts stale-until-REFRESH behavior for its TTL setting. These tests do not cover the cache-disable/expiry mismatch or an external schema change followed by Doris INSERT (inline findings).
- Scope: The production edit is confined to one lookup method, with corresponding unit and regression changes. The behavior affects downstream snapshot pinning and writer schema binding.
- Concurrency and lifecycle: The connector's table, latest-snapshot, and schema caches have distinct lifetimes. Shared table-scope invalidation fences in-flight publication; statement handles can retain their pin for that statement. No new locks, threads, circular ownership, or static initialization dependencies were introduced.
- Configuration: No new option was added. Existing paimon.cache-enabled and paimon.cache.expire-after-write settings expose an uncoordinated snapshot-cache lifetime (P1). Explicit schema.cache.ttl-second=0 can expose the writer-schema mismatch (P2). No dynamic configuration update path changed.
- Compatibility and parallel paths: No wire fields, function symbols, storage formats, or gensrc definitions changed. I traced ordinary scans, explicit time travel, system/branch handles, table-option copies, REST partition planning, Doris DDL, writes, and REFRESH; no further independent issue was substantiated.
- Conditions and tests: The edited method adds no runtime condition. The new unit assertion covers table-cache hits and direct invalidation; the removed test covered the previous invalidation behavior. The regression's no-cache catalog sets the snapshot TTL to zero, so it misses the distinct disabled-table-cache setting. Test expectations match the cases written, but the two findings remain untested. No builds or tests were run because this review is static only.
- Observability: No new execution path or log/metric instrumentation was added; the identified failures manifest as old rows or an Unknown column error. No separate observability defect was substantiated.
- Transactions, persistence, and writes: No EditLog format or replay behavior changed. Successful Doris mutation and refresh paths invalidate connector scope. A proposed post-commit edit-log failure was dismissed: the examined journal failure paths exit or throw rather than returning a healthy FE that serves a stale next query. The retained target Table can still bind an obsolete INSERT schema after an external ALTER (P2).
- FE/BE variables and performance: No FE-to-BE variable was added. Avoiding invalidation on each repeated lookup reduces unnecessary table reloads, but snapshot pins must be coordinated with the table cache (P1). No other substantiated CPU or memory regression was found.
The final changed-file and candidate sweep is complete. The accepted inline set is P1 and P2; overlapping cache candidates were merged, and no existing blocking comment IDs apply.
| // Invalidate that inner entry before reloading, otherwise REFRESH or outer-cache expiry can | ||
| // immediately repopulate Doris with the same stale bucket, path, or table options. | ||
| catalog.invalidateTable(identifier); | ||
| // Every metadata, scan and write lookup lands here, several times per statement. The wrapped |
There was a problem hiding this comment.
[P1] Keep the snapshot pin in step with the table cache. With paimon.cache-enabled=false, the table is reloaded on every lookup but PaimonLatestSnapshotCache still holds the prior snapshot id for its default 24-hour access TTL. Likewise, when a configured paimon.cache.expire-after-write reloads the table, the old snapshot id survives; beginQuerySnapshot keeps pinning scans to it, and frequent reads can extend that staleness indefinitely after an external INSERT. The removed invalidation previously cleared both entries. Disable or expire the snapshot cache whenever the table cache is disabled or expires, and cover both settings with external-write reads.
There was a problem hiding this comment.
The two lifetimes were already separate before this PR, and branch-4.x has the same staleness. Since the connector cutover (#64304) the latest-snapshot pin has followed meta.cache.paimon.table.ttl-second (an access TTL, 24 h by default). Setting it to 0 turns off both the table cache and the pin, which is what the no-cache catalog in test_paimon_table_meta_cache does. paimon.cache-enabled and paimon.cache.expire-after-* are Paimon's own catalog-cache options. #67996 applied them to the Doris table cache when it replaced the SDK CachingCatalog, but they never governed the pin. On branch-4.x they configure only the SDK CachingCatalog (4.x even turns it off under a weight limit). Doris keeps serving its cached snapshot there until the meta.cache.paimon.table TTL or REFRESH, and that TTL is access-based too, so frequent reads keep it alive there as well.
The per-lookup invalidation added by #67395 hid this, but it also kept the cache from ever hitting (0 hits in 86 lookups for 7 SELECTs), which is the bug this PR fixes. Making paimon.cache-enabled also turn off the snapshot pin would give the option a meaning it does not have on branch-4.x, so I'd keep that out of this PR.
| catalog.invalidateTable(identifier); | ||
| // Every metadata, scan and write lookup lands here, several times per statement. The wrapped | ||
| // catalog is the Doris table cache (PaimonMetaCacheCatalog; the SDK CachingCatalog is disabled), | ||
| // so this must not invalidate: REFRESH, Doris writes and DDL invalidate through CatalogMetaCache. |
There was a problem hiding this comment.
[P2] Resolve the current schema for INSERT targets. After an external ALTER TABLE ... ADD COLUMN, this retained Table can still have the old schema().fields(). PaimonWritePlanProvider.getWriteColumns uses those fields for the statement's writer schema, so an INSERT naming the new column fails as Unknown column until table expiry or REFRESH, even when schema.cache.ttl-second=0 lets DESC show the new column. The previous per-lookup invalidation reloaded the target Table. Read the live target schema at the writer boundary, and cover external ALTER followed by Doris INSERT.
There was a problem hiding this comment.
This matches branch-4.x, where #67023 made the Doris metadata cache the owner of the table instances that writes use, so a column another engine adds becomes writable only after REFRESH TABLE or cache expiry there too. What differs on master is that getTableSchema reads the latest schema file rather than the cached table's rowType() (a stale rowType() kept old names after a RENAME), so with schema.cache.ttl-second=0 DESC can show the column before INSERT accepts it. That window ends at REFRESH TABLE or when the cached table expires, at most Paimon's cache.expire-after-write (30 minutes by default).
Reloading the table for each write would have to give every handle in the statement the same table version, since the write plan and the bound write columns both read the table carried by their handle. That is a change to the write path's metadata contract rather than to this PR's cache hits, so I'd rather not fold it in here.
TPC-H: Total hot run time: 28636 ms |
TPC-DS: Total hot run time: 150720 ms |
ClickBench: Total hot run time: 24.52 s |
FE Regression Coverage ReportIncrement line coverage |
FE UT Coverage ReportIncrement line coverage `` 🎉 |
What problem does this PR solve?
Issue Number: None
Related PR: #67023, #67395, #67996
Problem Summary:
In short. Since #67395, every Paimon table lookup on master clears that table's cached metadata before reloading it, so the Paimon metadata cache never hits. Every statement reloads the table and its latest snapshot from the remote catalog, and a catalog with caching enabled no longer serves its cached snapshot. This PR removes the invalidation and restores the regression expectation that #67395 changed.
Background. #67395 forward-ported branch-4.1 #67023, which keeps the Paimon write cache coherent. In branch-4.x, the Doris table cache sits above Paimon's SDK
CachingCatalog. On a Doris cache miss, #67023 invalidates the SDK entry before reloading, so the reload cannot hand back a stale SDK table. #67395 placed that invalidation inCatalogBackedPaimonCatalogOps.getTable. On master, however, #67996 had already replaced the SDKCachingCatalogwith the Doris-ownedPaimonMetaCacheCatalog, andPaimonCatalogFactoryalways disables the SDK wrapper. The catalog thatgetTablewraps is therefore the Doris cache itself.CachingCatalogPaimonMetaCacheCatalogonly (SDK wrapper disabled)getTableHandle, several times per statementThe problem, and what it cost.
test_paimon_table_meta_cacheto expect the rows Spark wrote without a REFRESH, and attributed this to Paimon 1.4. branch-4.2 runs the same Paimon 1.4.2, and the same test there still expects the cached rows.How this PR fixes it.
CatalogBackedPaimonCatalogOps.getTableno longer invalidates the table. Nothing is lost by this: REFRESH TABLE / CATALOG, Doris writes (refreshTableAfterExternalMutation) and DDL all invalidate throughCatalogMetaCache, which clears thePaimonMetaCacheCatalogentries, and no other layer caches the table.test_paimon_table_meta_cacheexpects the cached rows again before the REFRESH.PaimonTableOptionsTest#catalogOpsInvalidatesPaimonCacheBeforeReloadpinned the invalidation. It is replaced byPaimonMetaCacheCatalogTest#connectorLookupsHitTheTableCacheUntilTheTableIsInvalidated: repeated lookups throughCatalogBackedPaimonCatalogOpsload the table once, and a lookup after the table is invalidated loads it again.Results. Measured on a local FE + BE with a filesystem Paimon catalog, with a second catalog on the same warehouse playing the external writer:
Putting the invalidation back makes the new unit test fail. All 675 tests of
fe-connector-paimonpass.Release note
None
Check List (For Author)
Test
test_paimon_table_meta_cacheneeds the Paimon docker fixture and was not run locally.PaimonMetaCacheCatalogTest,PaimonTableOptionsTestand the rest offe-connector-paimon(675 tests) pass locally.meta.cache.paimon.table.ttl-second= 0); results in the table above.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)