Обсуждение: Fix comments on _bt_skiparray_strat_increment/decrement
Hi, The attached patch is a trivial change to fix the comments on _bt_skiparray_strat_increment() and _bt_skiparray_strat_decrement() so that they are consistent with the comments on _bt_skiparray_strat_adjust(). At least the comment on _bt_skiparray_strat_decrement() containts an obvious typo, since it mentions converting the high_compare key instead of the low_compare key. Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp>
Вложения
> On Dec 30, 2025, at 18:01, Yugo Nagata <nagata@sraoss.co.jp> wrote:
>
> Hi,
>
> The attached patch is a trivial change to fix the comments on
> _bt_skiparray_strat_increment() and _bt_skiparray_strat_decrement() so
> that they are consistent with the comments on _bt_skiparray_strat_adjust().
> At least the comment on _bt_skiparray_strat_decrement() containts an
> obvious typo, since it mentions converting the high_compare key instead of
> the low_compare key.
>
> Regards,
> Yugo Nagata
>
> --
> Yugo Nagata <nagata@sraoss.co.jp>
> <fix_comments_on_bt_skiparray_strat_dec_inc_func.patch>
Good catch. Looks like a copy/paste mistake.
The code snippet prove the 2 functions' header comments are wrong:
```
/*
* Convert skip array's < low_compare key into a <= key
*/
static void
_bt_skiparray_strat_increment(IndexScanDesc scan, ScanKey arraysk,
BTArrayKeyInfo *array)
{
…
if (RegProcedureIsValid(cmp_proc))
{
/* Transform > low_compare key into >= key */
fmgr_info(cmp_proc, &low_compare->sk_func);
low_compare->sk_argument = new_sk_argument;
low_compare->sk_strategy = BTGreaterEqualStrategyNumber;
}
```
And
```
/*
* Convert skip array's > low_compare key into a >= key
*/
static void
_bt_skiparray_strat_decrement(IndexScanDesc scan, ScanKey arraysk,
BTArrayKeyInfo *array)
{
…
if (RegProcedureIsValid(cmp_proc))
{
/* Transform < high_compare key into <= key */
fmgr_info(cmp_proc, &high_compare->sk_func);
high_compare->sk_argument = new_sk_argument;
high_compare->sk_strategy = BTLessEqualStrategyNumber;
}
```
I also think we can delete “a” from the header comments. “into a >= key”, where “a” is an article (meaning one), but
canbe easily read as a variable name. The code comments don’t use “a” after “into”.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On Wed, 31 Dec 2025 11:36:56 +0800 Chao Li <li.evan.chao@gmail.com> wrote: > > > > On Dec 30, 2025, at 18:01, Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > Hi, > > > > The attached patch is a trivial change to fix the comments on > > _bt_skiparray_strat_increment() and _bt_skiparray_strat_decrement() so > > that they are consistent with the comments on _bt_skiparray_strat_adjust(). > > At least the comment on _bt_skiparray_strat_decrement() containts an > > obvious typo, since it mentions converting the high_compare key instead of > > the low_compare key. > > > > Regards, > > Yugo Nagata > > > > -- > > Yugo Nagata <nagata@sraoss.co.jp> > > <fix_comments_on_bt_skiparray_strat_dec_inc_func.patch> > > Good catch. Looks like a copy/paste mistake. > > The code snippet prove the 2 functions' header comments are wrong: Thank you for your reviewing. > I also think we can delete “a” from the header comments. “into a >= key”, where “a” is an article (meaning one), but canbe easily read as a variable name. The code comments don’t use “a” after “into”. The existing comments are grammatically correct, but as you point out, removing the "a" might make them less confusing. However, the comments in _bt_skiparray_strat_adjust() and in _bt_preprocess_array_keys_final(), which call this function, also use "a" after "into". If we remove the "a" here, should we also update those comments for consistency? -- Yugo Nagata <nagata@sraoss.co.jp>
> On Dec 31, 2025, at 14:57, Yugo Nagata <nagata@sraoss.co.jp> wrote: > > On Wed, 31 Dec 2025 11:36:56 +0800 > Chao Li <li.evan.chao@gmail.com> wrote: > >> >> >>> On Dec 30, 2025, at 18:01, Yugo Nagata <nagata@sraoss.co.jp> wrote: >>> >>> Hi, >>> >>> The attached patch is a trivial change to fix the comments on >>> _bt_skiparray_strat_increment() and _bt_skiparray_strat_decrement() so >>> that they are consistent with the comments on _bt_skiparray_strat_adjust(). >>> At least the comment on _bt_skiparray_strat_decrement() containts an >>> obvious typo, since it mentions converting the high_compare key instead of >>> the low_compare key. >>> >>> Regards, >>> Yugo Nagata >>> >>> -- >>> Yugo Nagata <nagata@sraoss.co.jp> >>> <fix_comments_on_bt_skiparray_strat_dec_inc_func.patch> >> >> Good catch. Looks like a copy/paste mistake. >> >> The code snippet prove the 2 functions' header comments are wrong: > > Thank you for your reviewing. > >> I also think we can delete “a” from the header comments. “into a >= key”, where “a” is an article (meaning one), but canbe easily read as a variable name. The code comments don’t use “a” after “into”. > > The existing comments are grammatically correct, but as you point out, > removing the "a" might make them less confusing. However, the comments in > _bt_skiparray_strat_adjust() and in _bt_preprocess_array_keys_final(), which > call this function, also use "a" after "into". > > If we remove the "a" here, should we also update those comments for consistency? > > -- > Yugo Nagata <nagata@sraoss.co.jp> It’s probably better to leave them as-is and just fix the error, which should make the patch easier to get through. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
On Wed, 31 Dec 2025 15:18:06 +0800 Chao Li <li.evan.chao@gmail.com> wrote: > > > > On Dec 31, 2025, at 14:57, Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > On Wed, 31 Dec 2025 11:36:56 +0800 > > Chao Li <li.evan.chao@gmail.com> wrote: > > > >> > >> > >>> On Dec 30, 2025, at 18:01, Yugo Nagata <nagata@sraoss.co.jp> wrote: > >>> > >>> Hi, > >>> > >>> The attached patch is a trivial change to fix the comments on > >>> _bt_skiparray_strat_increment() and _bt_skiparray_strat_decrement() so > >>> that they are consistent with the comments on _bt_skiparray_strat_adjust(). > >>> At least the comment on _bt_skiparray_strat_decrement() containts an > >>> obvious typo, since it mentions converting the high_compare key instead of > >>> the low_compare key. > >>> > >>> Regards, > >>> Yugo Nagata > >>> > >>> -- > >>> Yugo Nagata <nagata@sraoss.co.jp> > >>> <fix_comments_on_bt_skiparray_strat_dec_inc_func.patch> > >> > >> Good catch. Looks like a copy/paste mistake. > >> > >> The code snippet prove the 2 functions' header comments are wrong: > > > > Thank you for your reviewing. > > > >> I also think we can delete “a” from the header comments. “into a >= key”, where “a” is an article (meaning one), butcan be easily read as a variable name. The code comments don’t use “a” after “into”. > > > > The existing comments are grammatically correct, but as you point out, > > removing the "a" might make them less confusing. However, the comments in > > _bt_skiparray_strat_adjust() and in _bt_preprocess_array_keys_final(), which > > call this function, also use "a" after "into". > > > > If we remove the "a" here, should we also update those comments for consistency? > > > > -- > > Yugo Nagata <nagata@sraoss.co.jp> > > It’s probably better to leave them as-is and just fix the error, which should make the patch easier to get through. Agreed Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp>