Allow Delta flush_metadata_cache after table creation #17174
Conversation
Support following scenario - table was checked via Trino, so Delta connector knows table doesn't exist and cached that (`CachingMetastore` caches also misses). - table is created externally - `flush_metadata_cache` is used so that table becomes accessible via Trino
findepi
requested review from
homar,
losipiuk,
ebyhr,
alexjo2144 and
findinpath
findepi
added
the
no-release-notes
findepi
force-pushed
the
findepi/allow-delta-flush-metadata-cache-after-table-creation-7221ed
branch
from
805d359 to
12e5845
Compare
| extendedStatisticsAccess.invalidateCache(tableLocation); | ||
| // This may insert into a cache, but this will get invalidated below. TODO fix Delta so that flush_metadata_cache doesn't have to read from metastore | ||
| Optional<Table> tableBeforeFlush = metastore.getTable(schemaName.get(), tableName.get()); | ||
| cachingHiveMetastore.ifPresent(caching -> caching.invalidateTable(schemaName.get(), tableName.get())); |
There was a problem hiding this comment.
Does invalidateTable throw something if you call it with a schema/table that isn't cached? Seems like we should be able to just call it blindly without calling getTable first.
There was a problem hiding this comment.
Does invalidateTable throw something if you call it with a schema/table that isn't cached?
it does not
Seems like we should be able to just call it blindly without calling getTable first.
we call getTable to know the location (pre-existing)
we invalidate after calling, so that the cache is empty in the end state.
| tableLocation.ifPresent(transactionLogAccess::invalidateCaches); | ||
| tableLocation.ifPresent(extendedStatisticsAccess::invalidateCache); |
There was a problem hiding this comment.
Maybe the cache key for these should be a tableName, tableLocation tuple? That way you could invalidate them by name
There was a problem hiding this comment.
i added TODO fix Delta so that flush_metadata_cache doesn't have to read from metastore for now.
would prefer to revisit this after #17092
| .skippingTypesCheck() // Delta has no parametric varchar | ||
| .matches("TABLE tpch.tiny.region"); | ||
|
|
||
| assertUpdate("DROP TABLE flush_metadata_after_table_created"); |
There was a problem hiding this comment.
nit:
| assertUpdate("DROP TABLE flush_metadata_after_table_created"); | |
| assertUpdate("DROP TABLE " + tableName) |
There was a problem hiding this comment.
missed this one. will resolve in #17214
findepi
deleted the
findepi/allow-delta-flush-metadata-cache-after-table-creation-7221ed
branch
Support following scenario
exist and cached that (
CachingMetastorecaches also misses).flush_metadata_cacheis used so that table becomes accessible viaTrino