andygrove commented on PR #6098: URL: https://github.com/apache/datafusion-comet/pull/6098#issuecomment-5780211220
Thanks for the quick follow-up. The eviction order fix is clearly right, and I like that the rotate keeps the cost to a handful of pointer moves. I want to push on the 1 MiB admission limit. I measured what actually trips it and it turns out to be column count rather than pathological field names. With plain `Int32` columns the estimate crosses 1 MiB at around 5,800 columns. With 30-character column names it is around 4,100. With struct columns of 10 `Utf8` subfields it is around 585 of them. Those are wide schemas but they are real ones. At that point the cache turns off for that schema and every block reparses. Building this locally in the `ci` profile, a 6,000 column by 1,000 row block decodes in 2152 us with this change. At 5,000 columns the same shape is 877 us warm against 1778 us cold, so the cache is worth about 2x right up to the cutoff. It does not amortize as rows grow either, since the parse scales with columns and only the body scales with rows. That is the win #5809 was merged for, and the guard removes it from precisely the schemas where a reparse costs the most. Would you consider making the limit a budget for the cache as a whole rather than a per entry admission test? An oversized schema would then displace other entries rather than being refused, which keeps the working schema cached in the case that matters while still bounding what a thread retains. As written the aggregate is `SCHEMA_CACHE_CAPACITY * SCHEMA_CACHE_ENTRY_RETAIN_LIMIT`, so 4 MiB per thread, and that seems worth stating in the code either way. If we do keep a refusal, could the doc comment on `SCHEMA_CACHE_ENTRY_RETAIN_LIMIT` say roughly how many columns 1 MiB buys? That matters more now that the sentence explaining the value of `SCRATCH_RETAIN_LIMIT` is gone, and someone later deciding whether to raise it has nothing to go on. A 2x decode regression with no log and no metric behind it is hard to diagnose from the outside. -- 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]
