yinli-systems commented on issue #24117: URL: https://github.com/apache/datafusion/issues/24117#issuecomment-5670512885
I'd like to pick this up as a staged implementation, provided nobody already has parallel code for #24117. I traced the current plan boundary and the related work (#24227, #24113, #24822, #25185). Two constraints seem important: 1. `enable_rle_to_dictionary` in #24227 affects inferred table / scan schema before physical optimization, while the aggregate shape and bottom-up NDV statistics are only fully visible in the physical plan. A physical rule therefore cannot safely just flip that global flag; selective native reads need an internal per-scan/per-column decision channel (or an earlier planning hook) and downstream schema handling. 2. NDV alone is not a safe cost model. #24822 shows that dictionary grouping can still lose for both low-cardinality single keys and high-cardinality long strings; #25185 improves the repeated shared-values case, but does not make arbitrary runtime encoding free. My proposed first reviewable slice is deliberately narrow: - establish an end-to-end benchmark matrix for plain strings vs native/pre-encoded dictionaries vs runtime casts; - cover low/high NDV, short/long values, batch size/count, reused vs changing dictionary values, and partial/final aggregation; - verify identical results and separately measure conversion, aggregate, total time, and memory; - use those results to define a conservative, opt-in policy for direct `Utf8`/`Utf8View` GROUP BY columns; - missing or unsuitable statistics => no rewrite; uncertain benefit => no rewrite; no correctness or visible output-schema change; idempotent decision; - stack the scan mechanism on #24227 and avoid duplicating the user threshold work already owned in #24113. For the implementation path, I plan to prototype a private column-selection mechanism on the Parquet scan rather than expose a new public allowlist immediately. The optimizer/planning integration would only select columns whose physical encoding and statistics make native dictionary materialization viable; it would not insert an unproven generic per-batch cast. @Rich-T-kid @kumarUjjawal does that boundary match the direction you had in mind, and is anyone already implementing #24117? If not, please assign it to me and I will post the benchmark evidence and a concrete plan diff before expanding the surface area. -- 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]
