Re: [PATCH] we have added support for box type in SP-GiST index

Here are my comments about the operator class implementation:

> + *    implementation of quad-4d tree over boxes for SP-GiST.

Isn't the whole thing actually 3D?

> +  * For example consider the case of intersection.

No need for a new line after this.

> +  * A quadrant has bounds, but sp-gist keeps only 4-d point (box) in inner nodes.

I couldn't get the term 4D point.  Maybe it means that we are using
box datatype as the prefix, but we are not treating them as boxes.

> + * We use traversalValue to calculate quadrant bounds from parent's quadrant
> + * bounds.

I couldn't understand this sentence.  We are using the parent's prefix
and the quadrant to generate the traversalValue.  I think this is the
crucial part of the opclass.  It would be nice to explain it more
clearly.

> +  * Portions Copyright (c) 1996-2015, PostgreSQL Global Development Group

2016.

> + *          src/backend/utils/adt/boxtype_spgist.c

Maybe we should name this file as geo_spgist.c to support other types
in the future.

> + #include "utils/builtins.h";

Extra ;.

> + #include "utils/datum.h"

I think this is unnecessary.

> + /* InfR type implements doubles and +- infinity */
> + typedef struct
> + {
> +    int         infFlag;
> +    double      val;
> + }   InfR;

Why do we need this?  Can't we store infinity in "double"?

> + /* wrap double to InfR */
> + static InfR
> + toInfR(double v, InfR * r)
> + {
> +    r->infFlag = NotInf;
> +    r->val = v;
> + }

This isn't returning the value.

> + typedef struct
> + {
> +    Range       range_x;
> +    Range       range_y;
> + }   Rectangle;

Wouldn't it be simpler to just using BOX instead of this?

> + static void
> + evalIRangeBox(const IRangeBox *range_box, const Range *range, const int half1,
> +              const int half2, IRangeBox *new_range_box)
>
> + static void
> + evalIRectBox(const IRectBox *rect_box, const Rectangle *centroid,
> +             const uint8 quadrant, IRectBox * new_rect_box)
>
> + inline static void
> + initializeUnboundedBox(IRectBox * rect_box)

Wouldn't it be simpler to palloc and return the value on those functions?

> + static int
> + intersect2D(const Range * range, const IRangeBox * range_box)

Wouldn't it be better to return "bool" on those functions.

> +    return ((p1 >= 0) && (p2 <= 0));

Extra parentheses.

> + i    const spgChooseIn *in = (spgChooseIn *) PG_GETARG_POINTER(0);
> + i    spgChooseOut *out = (spgChooseOut *) PG_GETARG_POINTER(1);

Many variables are defined with "const".  I am not sure they are all
that doesn't change.  If it applies to the pointer, "out" should also
not change in here.  Am I wrong?

> +    /*
> +     * Begin. This block evaluates the median of coordinates of boxes
> +     */

I would rather explain what the function does on the function header.

> +             memcpy(new_rect_box, rect_box, sizeof(IRectBox));

Do we really need to copy the traversalValues on allTheSame case.
Wouldn't it work if just the same value is passed for all of them.
The search shouldn't continue after allTheSame case.

> +         for (i = 0; flag && i < in->nkeys && flag; i++)

It checks flag two times.

> +            boxPointerToRectangle(
> +                DatumGetBoxP(in->scankeys[i].sk_argument),
> +                p_query_rect
> +            );

I don't think this matches the project coding style.

> +     int         flag = 1,

Wouldn't it be better as "bool"?

The regression tests for the remaining operators are still missing.