Re: [HACKERS] [PATCH]: fix bug in SP-GiST box_ops

On 10.03.2017 02:13, Tels wrote:

> I can't comment on the code, but the grammar on the comments caught my eye:
>> +/* Can any range from range_box does not extend higher than this argument? */
>> +static bool
>> +overLower2D(RangeBox *range_box, Range *query)
>> +{
>> +    return FPle(range_box->left.low, query->high) &&
>> +        FPle(range_box->right.low, query->high);
>> +}
> The sentence sounds quite garbled in English. I'm not entirely sure what
> it should be, but given the comment below "/* Can any range from range_box
> to be higher than this argument? */" maybe something like:
>
> /* Does any range from range_box extend to the right side of the query? */
>
> If used, an analog wording should be used for overHigher2D's comment like:
>
> /* Does any range from range_box extend to the left side of the query? */

I've changed comments as you proposed, but I've added missing "not" and left "Can":

/* Can any range from range_box not extend to the right side of the query? */
/* Can any range from range_box not extend to the left side of the query? */

See also comments on overhigher and overlower operators from documentation:

&<    Does not extend to the right of?
&>    Does not extend to the left of?

> Another question: Does it make sense to add the "minimal bad example for
> the '&<' case" as test case, too? After all, it should pass the test after
> the patch.

Yes, it will make sense, but the Kyotaro's test case doesn't work for me and
I still don't know how to force SP-GiST to create inner leaves without
inserting hundreds of rows.

-- 
Nikita Glukhov
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers