Hi, I have resolved all the comments/issues reported in this new patch-set.
Changes done by Ashutosh Bapat for splitting out create_grouping_paths() into separate functions so that partitionwise aggregate code will use them were based on my partitionwise aggregate changes. Those were like refactoring changes. And thus, I have refactored them separately and before any partitionwise changes (see 0005-Split-create_grouping_paths-and-Add-GroupPathExtraDa.patch). And then I have re-based all partitionwise changes over it including all fixes. The patch-set is complete now. But still, there is a scope of some comment improvements due to all these refactorings. I will work on it. Also, need to update few documentations and indentations etc. Will post those changes in next patch-set. But meanwhile, this patch-set is ready to review. On Tue, Mar 13, 2018 at 9:12 AM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > On Mon, Mar 12, 2018 at 7:49 PM, Jeevan Chalke > <jeevan.cha...@enterprisedb.com> wrote: > > > > > > On Mon, Mar 12, 2018 at 6:07 PM, Ashutosh Bapat > > <ashutosh.ba...@enterprisedb.com> wrote: > >> > >> On Fri, Mar 9, 2018 at 4:21 PM, Ashutosh Bapat > >> > We don't need UpperRelationKind member in that structure. That will be > provided by the RelOptInfo passed. > > The problem here is the extra information required for grouping is not > going to be the same for that needed for window aggregate and > certainly not for ordering. If we try to jam everything in the same > structure, it will become large with many members useless for a given > operation. A reader will not have an idea about which of them are > useful and which of them are not. So, instead we should try some > polymorphism. I think we can pass a void * to GetForeignUpperPaths() > and corresponding FDW hook knows what to cast it to based on the > UpperRelationKind passed. > Yep. Done this way. > > BTW, the patch has added an argument to GetForeignUpperPaths() but has > not documented the change in API. If we go the route of polymorphism, > we will need to document the mapping between UpperRelationKind and the > type of structure passed in. > Oops. Will do that in next patchset. Thanks for pointing out, I have missed this at first place it self. -- Jeevan Chalke Technical Architect, Product Development EnterpriseDB Corporation The Enterprise PostgreSQL Company
partition-wise-agg-v17.tar.gz
Description: application/gzip
