wangyong9999 commented on code in PR #284:
URL: https://github.com/apache/paimon-cpp/pull/284#discussion_r3940459653
##########
src/paimon/core/operation/file_store_scan.cpp:
##########
@@ -557,6 +557,13 @@ Result<bool> FileStoreScan::FilterManifestEntry(const
ManifestEntry& entry) cons
if (bucket_filter_ != std::nullopt && entry.Bucket() !=
bucket_filter_.value()) {
return false;
}
+ // Hash with the file's bucket count, never the current table's count. An
older schema
+ // may encode bucket keys differently, so leave those files to the
existing stats filter.
+ if (bucket_selector_ && entry.TotalBuckets() > 0 &&
+ entry.File()->schema_id == table_schema_->Id() &&
Review Comment:
This guard is new for the key-value path. Before this PR the KV scan pruned
by the inferred bucket regardless of `schema_id`, so a table that ever added a
column now loses primary-key bucket pruning for every file written before that
change, and a time-travel scan against an older snapshot loses it entirely. The
hash only depends on the bucket-key columns. `FilterByStats` in both scans
already reads the file's schema when the id differs; compare the bucket-key
field types there (memoized per schema id) and skip only when they differ.
Java's `BucketSelector` applies to every entry with no schema check at all, so
that would still be the stricter of the two.
##########
src/paimon/core/operation/file_store_scan.cpp:
##########
@@ -602,6 +609,19 @@ Status FileStoreScan::SplitAndSetFilter(const
std::vector<std::string>& partitio
}
}
bucket_filter_ = scan_filters->GetBucketFilter();
+ const auto& bucket_keys = table_schema_->BucketKeys();
+ if (predicates_ && !bucket_filter_ && core_options_.GetBucket() > 0 &&
!bucket_keys.empty()) {
Review Comment:
Removing the global filter also removes the two planning optimizations the
KV path had before this PR: the min/max-bucket skip in `FilterManifestFileMeta`
and the snapshot live-manifest-entry cache are both gated on `bucket_filter_`,
so an inferred point lookup now reads every manifest on every plan even with
`scan.manifest-entry-cache.max-snapshots` set. The cache part does not need the
rescale-unsafe global filter: key the cache by the inferred bucket under the
current count together with that count, have `ReadAndMergeBucketFileEntries`
keep entries whose `TotalBuckets()` differs from the current count in addition
to `Bucket() == bucket`, and let the selector filter those at entry level as it
already does. Manifest skipping can stay explicit-only. Without this, tables
that never rescaled pay a full manifest read on every lookup, which is most of
the planning cost this pruning is meant to remove.
--
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]
