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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
