On Tue, Mar 28, 2017 at 12:21 PM, Amit Kapila <amit.kapil...@gmail.com> wrote: > I think both way it can work. I feel there is no hard pressed need > that we should make the computation in sorting same as what you do > _hash_doinsert. In patch_B, I don't think new naming for variables is > good. > > Assert(!a->isnull1); > - hash1 = DatumGetUInt32(a->datum1) & state->hash_mask; > + bucket1 = DatumGetUInt32(a->datum1) % state->num_buckets; > > Can we use hash_mod instead of num_buckets and retain hash1 in above > code and similar other places?
Yes done renamed it to hash_mod. > > Few comments: > 1. > +#define BUCKETS_WITHIN_SP_GRP(sp_g, nphase) \ > + ((sp_g < SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE) ? \ > + (1 << (sp_g - 1)) : \ > + ((nphase) * ((1 << (sp_g - 1)) >> 2))) > > This will go wrong for split point group zero. In general, I feel if > you handle computation for split groups lesser than > SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE in the caller, then all your > macros will become much simpler and less error prone. Fixed, apart from SPLITPOINT_PHASE_TO_SPLITPOINT_GRP rest all macros only handle multi phase group. The SPLITPOINT_PHASE_TO_SPLITPOINT_GRP is used in one more place at expand index so thought kepeping it as it is is better. . > 2. > +#define BUCKET_TO_SPLITPOINT_GRP(num_bucket) (_hash_log2(num_bucket)) > > What is the use of such a define, can't we directly use _hash_log2 in > the caller? No real reason, just that NAMED macro appeared more readable than just _hash_log2. Now I have removed same. > -- > With Regards, > Amit Kapila. > EnterpriseDB: http://www.enterprisedb.com -- Thanks and Regards Mithun C Y EnterpriseDB: http://www.enterprisedb.com
yet_another_expand_hashbucket_efficiently_09.patch
Description: Binary data
sortbuild_hash_B_2.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
