Replace sync cache build with recorder operator#78
Conversation
The optimizer used to call BuildCacheForPredicate on cache miss, making the first query pay for an extra full table scan. It now injects a PhysicalCacheRecorder above LogicalGet; the recorder evaluates the predicate per chunk and merges task-local bitvectors into the store at Finalize. Add an observed_vectors watermark to RowGroupFilter to tell "observed empty" apart from "not yet observed".
| // at or above this index is unknown and must be scanned. | ||
| struct RowGroupFilter { | ||
| array<uint64_t, BITVECTOR_ARRAY_SIZE> matching_vectors = {}; | ||
| idx_t observed_vectors = 0; |
There was a problem hiding this comment.
I think the watermark management is not quite good for cache invalidation.
how about we store another bitmask to record if the vector is observed?
struct RowGroupFilter {
array<uint64_t, BITVECTOR_ARRAY_SIZE> matching_vectors = {};
array<uint64_t, BITVECTOR_ARRAY_SIZE> observed = {};Switch matching_vectors and observed from raw array<uint64_t, N> to std::bitset<VECTORS_PER_ROW_GROUP>, and update related tests.
| entry = make_shared_ptr<ConditionCacheEntry>(); | ||
| store->Upsert(context, key, entry); | ||
|
|
||
| // TODO: Also inject the recorder on a partial cache hit so the watermark can |
There was a problem hiding this comment.
I think we already removed the watermark?
| make_uniq<CacheExpressionFilter>(std::move(filter_expr), entry)); | ||
| } | ||
|
|
||
| void QueryConditionCacheOptimizer::InjectCacheRecorder(ClientContext &context, unique_ptr<LogicalOperator> &plan, |
There was a problem hiding this comment.
can you use plain words to explain the logic of this function, espically the rowid_chunk_idx and rowid_column_ids_pos
|
|
||
| auto query_state = input.context.registered_state->Get<CacheOptimizerQueryState>(CacheOptimizerQueryState::NAME); | ||
| if (!query_state || query_state->cache_apply_pending.empty()) { | ||
| if (!query_state || (query_state->cache_apply_pending.empty() && query_state->cache_recorder_pending.empty())) { |
There was a problem hiding this comment.
our disscusion is to keep keep the recorder anyway? and thus I think we can remove the two phase optimizer(query context)? because we aren't going to do any cache lookup thing when plan rewriting, we do all lookup and upsert things inside physical recorder operator and the condition cache filter expression when they are processing the incoming data chunk?
| auto first_idx = rowid_data.sel->get_index(0); | ||
| if (!rowid_data.validity.RowIsValid(first_idx)) { | ||
| return OperatorResultType::NEED_MORE_INPUT; | ||
| } |
There was a problem hiding this comment.
is this case possible? if this happened means no rows in the vector is valid, and I though all invalid vector wont flow into here?
peterxcli
left a comment
There was a problem hiding this comment.
We should think this through:
- When the recorder sees a vector that hasn’t been observed yet, it should upsert both the
observedandmatching_vectorsbitmasks. - Do we actually only need
observedper row group? The recorder could upsert all cache entries for every vector it sees, then mark that row group as observed. - The query cache expression filter should only skip a vector if that vector’s row group is marked as observed in the cache entry.
Closes: #75
Summary
This PR replaces the synchronous full-table build that the optimizer used to
run on cache miss with a
PhysicalCacheRecorderoperator that observes theuser's own query and builds the cache as a side-effect. The first query on
a new predicate no longer pays an extra full-table scan.
An
observed_vectorswatermark is added toRowGroupFilterso the cachecan distinguish "observed empty" (safe to prune) from "not yet observed"
(must scan). Recorder injection is narrow: it only runs on a full cache
miss. Partial refill and scan-skipped row groups are tracked as TODOs.
Operator Hierarchy
Cache miss — optimizer injects the recorder above
LogicalGet:Cache hit — no recorder, filter only:
Runtime flow for the cache-miss case: