kosiew commented on code in PR #23402:
URL: https://github.com/apache/datafusion/pull/23402#discussion_r4081239887
##########
datafusion/datasource/src/url.rs:
##########
@@ -399,29 +393,32 @@ async fn list_with_cache<'b>(
.map(|res| res.map_err(|e|
DataFusionError::ObjectStore(Box::new(e))))
.boxed()),
Some(cache) => {
- // Build the filter prefix (only Some if prefix was requested)
let filter_prefix = prefix.is_some().then(|| full_prefix.clone());
let table_scoped_base_path = TableScopedPath {
table: table_ref.cloned(),
path: table_base_path.clone(),
};
+ let table_scoped_list_path = TableScopedPath {
Review Comment:
Optional extra coverage: it might be useful to cache `region=US/q1` first,
then request `region=US` when another file also exists under that prefix. The
existing prefix-to-root test exercises the same lookup branch, so I don't think
this is blocking, but it could give us a little more confidence that a narrower
cached prefix is never treated as satisfying a broader request.
##########
datafusion/catalog-listing/src/table.rs:
##########
@@ -823,11 +823,21 @@ impl ListingTable {
// Invalidate cache entries for this table if they exist
if let Some(lfc) =
state.runtime_env().cache_manager.get_list_files_cache() {
- let key = TableScopedPath {
- table: table_path.get_table_ref().clone(),
- path: table_path.prefix().clone(),
- };
- let _ = lfc.remove(&key);
+ if let Some(table_ref) = table_path.get_table_ref() {
+ lfc.drop_table_entries(table_ref)?;
+ } else {
+ let table_prefix = table_path.prefix();
+ let keys: Vec<_> = lfc
+ .list_entries()
+ .into_keys()
+ .filter(|key| {
+ key.table.is_none() &&
key.path.prefix_matches(table_prefix)
Review Comment:
I think there is still a stale-cache case here for overlapping unscoped
`ListingTable`s. We currently remove entries at or below the written table's
prefix, but not an ancestor entry.
For example, suppose `table/` is cached, then we insert through an unscoped
table rooted at `table/region=US/`. A later scan through `table/` will check
its cached base entry first, so it can miss the newly inserted file. An insert
through the same `table/` URL is fine because `prefix_matches` includes
equality.
Could we invalidate unscoped cache entries whose paths overlap the written
prefix in both directions, and add a regression test for the
root-table/child-table case?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
