Re: jsonpath

Hi,

On 2019-01-29 04:00:19 +0300, Alexander Korotkov wrote:
> +/*
> + * Make datetime type from 'date_txt' which is formated at argument 'fmt'.
> + * Actual datatype (returned in 'typid', 'typmod') is determined by
> + * presence of date/time/zone components in the format string.
> + */
> +Datum
> +to_datetime(text *date_txt, text *fmt, char *tzname, bool strict, Oid *typid,
> + int32 *typmod, int *tz)

Given other to_<type> functions being SQL callable, I'm not a fan of the
naming here.

> +{
> + struct pg_tm tm;
> + fsec_t fsec;
> + int fprec = 0;
> + int flags;
> +
> + do_to_timestamp(date_txt, fmt, strict, &tm, &fsec, &fprec, &flags);
> +
> + *typmod = fprec ? fprec : -1; /* fractional part precision */
> + *tz = 0;
> +
> + if (flags & DCH_DATED)
> + {
> + if (flags & DCH_TIMED)
> + {
> + if (flags & DCH_ZONED)
> + {
> + TimestampTz result;
> +
> + if (tm.tm_zone)
> + tzname = (char *) tm.tm_zone;
> +
> + if (tzname)
> + {
> + int dterr = DecodeTimezone(tzname, tz);
> +
> + if (dterr)
> + DateTimeParseError(dterr, tzname, "timestamptz");

Do we really need 6/7 indentation levels in new code?

> +jsonpath_scan.c: FLEXFLAGS = -CF -p -p

Why -CF, and why is -p repeated?

> - JsonEncodeDateTime(buf, val, DATEOID);
> + JsonEncodeDateTime(buf, val, DATEOID, NULL);

ISTM API changes like this ought to be done in a separate patch, to ease
review.

> }
>
> +
> /*
> * Compare two jbvString JsonbValue values, a and b.
> *

Random WS change.

> +/*****************************INPUT/OUTPUT*********************************/

Why do we need this much code to serialize data that we initially got in
serialized manner? Couldn't we just keep the original around?

> +Datum
> +jsonpath_in(PG_FUNCTION_ARGS)
> +{

No binary input support? I'd suggest adding that, but keeping the
representation the same. Otherwise just adds unnecesary complexity for
driver authors wishing to use the binar protocol.

> +/********************Support functions for JsonPath**************************/
> +
> +/*
> + * Support macroses to read stored values
> + */

s/macroses/macros/

> +++ b/src/backend/utils/adt/jsonpath_exec.c

Gotta say, I'm unenthusiastic about yet another execution engine in some
PG subsystem.

> @@ -0,0 +1,2776 @@
> +/*-------------------------------------------------------------------------
> + *
> + * jsonpath_exec.c
> + * Routines for SQL/JSON path execution.
> + *
> + * Copyright (c) 2019, PostgreSQL Global Development Group
> + *
> + * IDENTIFICATION
> + * src/backend/utils/adt/jsonpath_exec.c
> + *
> + *-------------------------------------------------------------------------
> + */

Somewhere there needs to be higher level documentation explaining how
this stuff is supposed to work on a code level.

> +
> +/* strict/lax flags is decomposed into four [un]wrap/error flags */
> +#define jspStrictAbsenseOfErrors(cxt) (!(cxt)->laxMode)
> +#define jspAutoUnwrap(cxt) ((cxt)->laxMode)
> +#define jspAutoWrap(cxt) ((cxt)->laxMode)
> +#define jspIgnoreStructuralErrors(cxt) ((cxt)->ignoreStructuralErrors)
> +#define jspThrowErrors(cxt) ((cxt)->throwErrors)

What's the point?

> +#define ThrowJsonPathError(code, detail) \
> + ereport(ERROR, \
> + (errcode(ERRCODE_ ## code), \
> + errmsg(ERRMSG_ ## code), \
> + errdetail detail))
> +
> +#define ThrowJsonPathErrorHint(code, detail, hint) \
> + ereport(ERROR, \
> + (errcode(ERRCODE_ ## code), \
> + errmsg(ERRMSG_ ## code), \
> + errdetail detail, \
> + errhint hint))

These seem ill-advised, just making it harder to understand the code,
and grepping for it, without actually meaningfully simplifying anything.

> +/*
> + * Find value of jsonpath variable in a list of passing params
> + */

What is that comment supposed to mean?

> +/*
> + * Get the type name of a SQL/JSON item.
> + */
> +static const char *
> +JsonbTypeName(JsonbValue *jb)
> +{

Wasn't there pretty similar infrastructure elsewhere?

> +/*
> + * Cross-type comparison of two datetime SQL/JSON items. If items are
> + * uncomparable, 'error' flag is set.
> + */

Sounds like it'd not raise an error, but it does in a bunch of scenarios.

> @@ -206,6 +206,22 @@ Section: Class 22 - Data Exception
> 2200N E ERRCODE_INVALID_XML_CONTENT invalid_xml_content
> 2200S E ERRCODE_INVALID_XML_COMMENT invalid_xml_comment
> 2200T E ERRCODE_INVALID_XML_PROCESSING_INSTRUCTION invalid_xml_processing_instruction
> +22030 E ERRCODE_DUPLICATE_JSON_OBJECT_KEY_VALUE duplicate_json_object_key_value
> +22031 E ERRCODE_INVALID_ARGUMENT_FOR_JSON_DATETIME_FUNCTION invalid_argument_for_json_datetime_function
> +22032 E ERRCODE_INVALID_JSON_TEXT invalid_json_text
> +22033 E ERRCODE_INVALID_JSON_SUBSCRIPT invalid_json_subscript
> +22034 E ERRCODE_MORE_THAN_ONE_JSON_ITEM more_than_one_json_item
> +22035 E ERRCODE_NO_JSON_ITEM no_json_item
> +22036 E ERRCODE_NON_NUMERIC_JSON_ITEM non_numeric_json_item
> +22037 E ERRCODE_NON_UNIQUE_KEYS_IN_JSON_OBJECT non_unique_keys_in_json_object
> +22038 E ERRCODE_SINGLETON_JSON_ITEM_REQUIRED singleton_json_item_required
> +22039 E ERRCODE_JSON_ARRAY_NOT_FOUND json_array_not_found
> +2203A E ERRCODE_JSON_MEMBER_NOT_FOUND json_member_not_found
> +2203B E ERRCODE_JSON_NUMBER_NOT_FOUND json_number_not_found
> +2203C E ERRCODE_JSON_OBJECT_NOT_FOUND object_not_found
> +2203F E ERRCODE_JSON_SCALAR_REQUIRED json_scalar_required
> +2203D E ERRCODE_TOO_MANY_JSON_ARRAY_ELEMENTS too_many_json_array_elements
> +2203E E ERRCODE_TOO_MANY_JSON_OBJECT_MEMBERS
> too_many_json_object_members

Are all of these invented by us?

Greetings,

Andres Freund