On Wed, Sep 11, 2024 at 10:52 AM Tender Wang <tndrw...@gmail.com> wrote: > 1. In make_one_rel(), we have the below codes: > /* > * Build grouped base relations for each base rel if possible. > */ > setup_base_grouped_rels(root); > > As far as I know, each base rel only has one grouped base relation, if > possible. > The comments may be changed to "Build a grouped base relation for each base > rel if possible."
Yeah, each base rel has only one grouped rel. However, there is a comment nearby stating 'consider_parallel flags for each base rel', which confuses me about whether it should be singular or plural in this context. Perhaps someone more proficient in English could clarify this. > 2. According to the comments of generate_grouped_paths(), we may generate > paths for a grouped > relation on top of paths of join relation. So the ”rel_plain" argument in > generate_grouped_paths() may be > confused. "plain" usually means "base rel" . How about Re-naming rel_plain to > input_rel? I don't think 'plain relation' necessarily means 'base relation'. In this context I think it can mean 'non-grouped relation'. But maybe I'm wrong. > 3. In create_partial_grouping_paths(), The partially_grouped_rel could have > been already created due to eager > aggregation. If partially_grouped_rel exists, its reltarget has been > created. So do we need below logic? > > /* > * Build target list for partial aggregate paths. These paths cannot just > * emit the same tlist as regular aggregate paths, because (1) we must > * include Vars and Aggrefs needed in HAVING, which might not appear in > * the result tlist, and (2) the Aggrefs must be set in partial mode. > */ > partially_grouped_rel->reltarget = > make_partial_grouping_target(root, grouped_rel->reltarget, > extra->havingQual); Yeah, maybe we can avoid building the target list here for partially_grouped_rel that is generated by eager aggregation. Thanks Richard
