On Tue, Mar 28, 2017 at 12:21 PM, Amit Kapila <amit.kapila16@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
