LuciferYang commented on PR #68018: URL: https://github.com/apache/doris/pull/68018#issuecomment-5747709933
Thanks for the re-review. Agreed there is no new inline finding here; the three P1s it re-raises (r4017097815, r4025665398, r4032777651) are the same placeholder-index exposure, and I want to state how it is fenced rather than leave the deferral implicit. This PR is not meant to merge independently. It is ordered behind #67995, which moves placeholder handling into `Segment::new_index_iterator` (and the zone-map builders and the pushed-agg gate), keyed on the column's schema ordinal rather than on which reader happens to be cached. Until #67995 is in, "this head is not independently safe to merge" is accurate and expected, not an oversight. The reason the guard is not in this diff is the same evidence those three findings provide. A reader-type check cannot carry the invariant: eviction (r4032777651) and a concurrent bare miss (r4025665398) both rebuild a physical reader for a placeholder column, and the bare index path passes no constant, so the check is bypassed. That is why I removed the `ConstantColumnReader::new_index_iterator` override. Residency and arrival order are not a place to store a correctness property; the schema is. So this PR stays scoped to the cache contract: a request carrying a constant is not dropped on a hit, and the entry is replaced in place. That part its two unit tests cover on their own. #67995 lands first, then this one. -- 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]
