some of the switch->default, default don't have ``break``.
The compiler doesn't require them, but I see that we do use them in a lot of places, so I'll incorporate this.
+ for (int i = 0; i < nitems; i++) + { + MVNDistinctItem *item = parse_state.distinct_items->elements[i].ptr_value;
exposing the ptr_value seems not a good idea, we can foreach_ptr the attached patch using foreach_ptr.
I didn't like this because it makes getting the index value harder, and the index value is needed in lots of places. Instead I used list_nth() and list_nth_int().
in function pg_ndistinct_in some errsave can change to ereturn. (I didn't do this part, though).
-0.25
There's something that I like about the consistency of errsave() followed by a return that makes it clear that "the function ends here" that I don't get from ereturn().
What I would really like, is a way to generate unique (but translated) errdetail values, and move the errsave/ereturn to after the switch statement.
+ /* + * The attnum cannot be zero a negative number beyond the number of the + * possible expressions. + */ + if (attnum == 0 || attnum < (0-STATS_MAX_DIMENSIONS)) + { + errsave(parse->escontext, + errcode(ERRCODE_INVALID_TEXT_REPRESENTATION), + errmsg("malformed pg_ndistinct: \"%s\"", parse->str), + errdetail("Invalid \"%s\" element: %d.", + PG_NDISTINCT_KEY_ATTRIBUTES, attnum)); + return JSON_SEM_ACTION_FAILED; + } This part had no coverage tests, so I added a few.
+1
as mentioned before + errsave(parse->escontext, + errcode(ERRCODE_INVALID_TEXT_REPRESENTATION), + errmsg("malformed pg_ndistinct: \"%s\"", parse->str), + errdetail("The \"%s\" key must contain an array of at least %d " + "and no more than %d attributes.", + PG_NDISTINCT_KEY_NDISTINCT, 2, STATS_MAX_DIMENSIONS)); here PG_NDISTINCT_KEY_NDISTINCT, should be PG_NDISTINCT_KEY_ATTRIBUTES.
+1
Did similar things to pg_dependencies.
A few small comments.
1. Minor typo fixes:
In pg_dependencies_in: "pg_dependencies parssing claims..." could be corrected to "pg_dependencies parsing claims..." In ndistinct_array_end: "must be an non-empty array" might be better as "must be a non-empty array" In the comment for dependencies_object_field_start: "depeendency" appears to be a typo for "dependency"
2.Code maintainability suggestion:
I noticed the string "malformed pg_dependencies: "%s"" is used repeatedly throughout the code. Would you consider defining this as a macro? This could reduce duplication and make future updates easier.
3.Memory management observation:
Regarding item_attnum_list, while PostgreSQL's memory context mechanism handles cleanup, explicitly freeing the allocated memory after use might improve code clarity.
These are all minor points - the implementation looks solid overall. Thank you for your work on this feature!
