Re: Cross-type index comparison support in contrib/btree_gin

Arseniy Mukhin <arseniy(dot)mukhin(dot)dev(at)gmail(dot)com> writes:
> I tried the patch and it compiles and works as expected. Several minor
> things I noticed:

Thanks for looking at it!

> 1) btree_gin.c

> case BTEqualStrategyNumber:
> if (cmp > 0)
> res = -1; /* keep scanning */
> else if (cmp == 0)
> res = 0;
> else
> res = 1; /* end scan */
> break;

> I think the code is correct, but do we need to continue scanning here?
> I can't think of an example where we have cmp == 0 after cmp > 0.
> Maybe we can use cmp != 0 as a stopping condition?

No, I think you're reading the code backward. cmp > 0 means the
current index entry is less than the query's search value, so we
need to keep scanning forward to see if there's any entries that
match. We can stop when we get to larger entries, with cmp < 0.

I found this sign convention confusing too, and considered reversing
the comparison call so that the "cmp" comparisons in this function
would all flip. But I felt that such a change maybe doesn't belong
in this patch. Also I wasn't sure other people would agree that
it'd be an improvement --- the original code author, for one,
presumably finds this natural.

> 2) btree_gin.c

> switch (data->strategy & 7)
> {
> case BTLessStrategyNumber:

> There are two places where we extract btree_start from the input
> strategy. Maybe we can store the extracted value in QueryInfo? Or
> create macros to avoid code duplication.

Hardly seems worth maintaining an extra field to get rid of a
bit-masking operation. But maybe a macro would improve readability.

> 3) float4.sql

> -- Check endpoint and out-of-range cases

> INSERT INTO test_float4 VALUES ('NaN'), ('Inf'), ('-Inf');

> It seems that this test data is in the pending list during the test.
> Not sure if we want them to be in the entry tree or not? The same with
> date.sql and timestamp.sql.

Good point, it probably makes more sense to force the data into
the entry tree. It's not the charter of these tests to verify
that the pending-list works properly.

>> * Query value falls between possible values of the index type
>> (possible in float8->float4 or timestamp->date cases, for example).
>> We can just use our usual conversion rules, though. The critical
>> observation here is that it does not matter whether the conversion
>> rounds to the next lower or next higher possible value.

> I agree with the statements. It's quite a tricky part as for me,
> probably it's better to add tests for this special case as it's done
> for 'out of range' cases. FWIW while testing the patch I wrote some
> tests for these rounding cases, I'm ready to add it to the patchset.

Makes sense. Do you want to prepare the next patch version, then?

regards, tom lane