From 760f3c043ad4ee622b596d005ec31bb5e0322c4a Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 25 Jan 2013 00:19:18 -0500 Subject: [PATCH] Fix concat() and format() to handle VARIADIC-labeled arguments correctly. Previously, the VARIADIC labeling was effectively ignored, but now these functions act as though the array elements had all been given as separate arguments. Pavel Stehule --- doc/src/sgml/func.sgml | 24 +++- doc/src/sgml/xfunc.sgml | 11 +- src/backend/utils/adt/varlena.c | 198 +++++++++++++++++++++++++---- src/test/regress/expected/text.out | 89 ++++++++++++- src/test/regress/sql/text.sql | 23 +++- 5 files changed, 310 insertions(+), 35 deletions(-) diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 35c7f75eab..e9dbcb9f8a 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -1394,7 +1394,8 @@ text - Concatenate all arguments. NULL arguments are ignored. + Concatenate the text representations of all the arguments. + NULL arguments are ignored. concat('abcde', 2, NULL, 22) abcde222 @@ -1411,8 +1412,8 @@ text - Concatenate all but first arguments with separators. The first - parameter is used as a separator. NULL arguments are ignored. + Concatenate all but the first argument with separators. The first + argument is used as the separator string. NULL arguments are ignored. concat_ws(',', 'abcde', 2, NULL, 22) abcde,2,22 @@ -1522,8 +1523,9 @@ text - Format a string. This function is similar to the C function - sprintf; but only the following conversion specifications + Format arguments according to a format string. + This function is similar to the C function + sprintf, but only the following conversion specifications are recognized: %s interpolates the corresponding argument as a string; %I escapes its argument as an SQL identifier; %L escapes its argument as an @@ -2033,6 +2035,18 @@ + + The concat, concat_ws and + format functions are variadic, so it is possible to + pass the values to be concatenated or formatted as an array marked with + the VARIADIC keyword (see ). The array's elements are + treated as if they were separate ordinary arguments to the function. + If the variadic array argument is NULL, concat + and concat_ws return NULL, but + format treats a NULL as a zero-element array. + + See also the aggregate function string_agg in . diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index 85539feb0d..4fb42842c6 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -3153,6 +3153,10 @@ CREATE OR REPLACE FUNCTION retcomposite(IN integer, IN integer, fcinfo->flinfo. The parameter argnum is zero based. get_call_result_type can also be used as an alternative to get_fn_expr_rettype. + There is also get_fn_expr_variadic, which can be used to + find out whether the call contained an explicit VARIADIC + keyword. This is primarily useful for VARIADIC "any" + functions, as described below. @@ -3229,7 +3233,12 @@ CREATE FUNCTION make_array(anyelement) RETURNS anyarray as happens with normal variadic functions; they will just be passed to the function separately. The PG_NARGS() macro and the methods described above must be used to determine the number of actual - arguments and their types when using this feature. + arguments and their types when using this feature. Also, users of such + a function might wish to use the VARIADIC keyword in their + function call, with the expectation that the function would treat the + array elements as separate arguments. The function itself must implement + that behavior if wanted, after using get_fn_expr_variadic to + detect that the actual argument was marked with VARIADIC. diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c index 95e41bf30a..e69b7dd3e6 100644 --- a/src/backend/utils/adt/varlena.c +++ b/src/backend/utils/adt/varlena.c @@ -76,12 +76,12 @@ static bytea *bytea_substring(Datum str, bool length_not_specified); static bytea *bytea_overlay(bytea *t1, bytea *t2, int sp, int sl); static StringInfo makeStringAggState(FunctionCallInfo fcinfo); -void text_format_string_conversion(StringInfo buf, char conversion, - Oid typid, Datum value, bool isNull); - +static void text_format_string_conversion(StringInfo buf, char conversion, + FmgrInfo *typOutputInfo, + Datum value, bool isNull); static Datum text_to_array_internal(PG_FUNCTION_ARGS); static text *array_to_text_internal(FunctionCallInfo fcinfo, ArrayType *v, - char *fldsep, char *null_string); + const char *fldsep, const char *null_string); /***************************************************************************** @@ -3451,7 +3451,7 @@ array_to_text_null(PG_FUNCTION_ARGS) */ static text * array_to_text_internal(FunctionCallInfo fcinfo, ArrayType *v, - char *fldsep, char *null_string) + const char *fldsep, const char *null_string) { text *result; int nitems, @@ -3791,11 +3791,12 @@ string_agg_finalfn(PG_FUNCTION_ARGS) /* * Implementation of both concat() and concat_ws(). * - * sepstr/seplen describe the separator. argidx is the first argument - * to concatenate (counting from zero). + * sepstr is the separator string to place between values. + * argidx identifies the first argument to concatenate (counting from zero). + * Returns NULL if result should be NULL, else text value. */ static text * -concat_internal(const char *sepstr, int seplen, int argidx, +concat_internal(const char *sepstr, int argidx, FunctionCallInfo fcinfo) { text *result; @@ -3803,6 +3804,48 @@ concat_internal(const char *sepstr, int seplen, int argidx, bool first_arg = true; int i; + /* + * concat(VARIADIC some-array) is essentially equivalent to + * array_to_text(), ie concat the array elements with the given separator. + * So we just pass the case off to that code. + */ + if (get_fn_expr_variadic(fcinfo->flinfo)) + { + Oid arr_typid; + ArrayType *arr; + + /* Should have just the one argument */ + Assert(argidx == PG_NARGS() - 1); + + /* concat(VARIADIC NULL) is defined as NULL */ + if (PG_ARGISNULL(argidx)) + return NULL; + + /* + * Non-null argument had better be an array. The parser doesn't + * enforce this for VARIADIC ANY functions (maybe it should?), so that + * check uses ereport not just elog. + */ + arr_typid = get_fn_expr_argtype(fcinfo->flinfo, argidx); + if (!OidIsValid(arr_typid)) + elog(ERROR, "could not determine data type of concat() input"); + + if (!OidIsValid(get_element_type(arr_typid))) + ereport(ERROR, + (errcode(ERRCODE_DATATYPE_MISMATCH), + errmsg("VARIADIC argument must be an array"))); + + /* OK, safe to fetch the array value */ + arr = PG_GETARG_ARRAYTYPE_P(argidx); + + /* + * And serialize the array. We tell array_to_text to ignore null + * elements, which matches the behavior of the loop below. + */ + return array_to_text_internal(fcinfo, arr, sepstr, NULL); + } + + /* Normal case without explicit VARIADIC marker */ initStringInfo(&str); for (i = argidx; i < PG_NARGS(); i++) @@ -3818,7 +3861,7 @@ concat_internal(const char *sepstr, int seplen, int argidx, if (first_arg) first_arg = false; else - appendBinaryStringInfo(&str, sepstr, seplen); + appendStringInfoString(&str, sepstr); /* call the appropriate type output function, append the result */ valtype = get_fn_expr_argtype(fcinfo->flinfo, i); @@ -3842,7 +3885,12 @@ concat_internal(const char *sepstr, int seplen, int argidx, Datum text_concat(PG_FUNCTION_ARGS) { - PG_RETURN_TEXT_P(concat_internal("", 0, 0, fcinfo)); + text *result; + + result = concat_internal("", 0, fcinfo); + if (result == NULL) + PG_RETURN_NULL(); + PG_RETURN_TEXT_P(result); } /* @@ -3852,16 +3900,18 @@ text_concat(PG_FUNCTION_ARGS) Datum text_concat_ws(PG_FUNCTION_ARGS) { - text *sep; + char *sep; + text *result; /* return NULL when separator is NULL */ if (PG_ARGISNULL(0)) PG_RETURN_NULL(); + sep = text_to_cstring(PG_GETARG_TEXT_PP(0)); - sep = PG_GETARG_TEXT_PP(0); - - PG_RETURN_TEXT_P(concat_internal(VARDATA_ANY(sep), VARSIZE_ANY_EXHDR(sep), - 1, fcinfo)); + result = concat_internal(sep, 1, fcinfo); + if (result == NULL) + PG_RETURN_NULL(); + PG_RETURN_TEXT_P(result); } /* @@ -3959,11 +4009,73 @@ text_format(PG_FUNCTION_ARGS) const char *end_ptr; text *result; int arg = 0; + bool funcvariadic; + int nargs; + Datum *elements = NULL; + bool *nulls = NULL; + Oid element_type = InvalidOid; + Oid prev_type = InvalidOid; + FmgrInfo typoutputfinfo; /* When format string is null, returns null */ if (PG_ARGISNULL(0)) PG_RETURN_NULL(); + /* If argument is marked VARIADIC, expand array into elements */ + if (get_fn_expr_variadic(fcinfo->flinfo)) + { + Oid arr_typid; + ArrayType *arr; + int16 elmlen; + bool elmbyval; + char elmalign; + int nitems; + + /* Should have just the one argument */ + Assert(PG_NARGS() == 2); + + /* If argument is NULL, we treat it as zero-length array */ + if (PG_ARGISNULL(1)) + nitems = 0; + else + { + /* + * Non-null argument had better be an array. The parser doesn't + * enforce this for VARIADIC ANY functions (maybe it should?), so + * that check uses ereport not just elog. + */ + arr_typid = get_fn_expr_argtype(fcinfo->flinfo, 1); + if (!OidIsValid(arr_typid)) + elog(ERROR, "could not determine data type of format() input"); + + if (!OidIsValid(get_element_type(arr_typid))) + ereport(ERROR, + (errcode(ERRCODE_DATATYPE_MISMATCH), + errmsg("VARIADIC argument must be an array"))); + + /* OK, safe to fetch the array value */ + arr = PG_GETARG_ARRAYTYPE_P(1); + + /* Get info about array element type */ + element_type = ARR_ELEMTYPE(arr); + get_typlenbyvalalign(element_type, + &elmlen, &elmbyval, &elmalign); + + /* Extract all array elements */ + deconstruct_array(arr, element_type, elmlen, elmbyval, elmalign, + &elements, &nulls, &nitems); + } + + nargs = nitems + 1; + funcvariadic = true; + } + else + { + /* Non-variadic case, we'll process the arguments individually */ + nargs = PG_NARGS(); + funcvariadic = false; + } + /* Setup for main loop. */ fmt = PG_GETARG_TEXT_PP(0); start_ptr = VARDATA_ANY(fmt); @@ -4062,26 +4174,54 @@ text_format(PG_FUNCTION_ARGS) } /* Not enough arguments? Deduct 1 to avoid counting format string. */ - if (arg > PG_NARGS() - 1) + if (arg > nargs - 1) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("too few arguments for format"))); + /* Get the value and type of the selected argument */ + if (!funcvariadic) + { + value = PG_GETARG_DATUM(arg); + isNull = PG_ARGISNULL(arg); + typid = get_fn_expr_argtype(fcinfo->flinfo, arg); + } + else + { + value = elements[arg - 1]; + isNull = nulls[arg - 1]; + typid = element_type; + } + if (!OidIsValid(typid)) + elog(ERROR, "could not determine data type of format() input"); + + /* + * Get the appropriate typOutput function, reusing previous one if + * same type as previous argument. That's particularly useful in the + * variadic-array case, but often saves work even for ordinary calls. + */ + if (typid != prev_type) + { + Oid typoutputfunc; + bool typIsVarlena; + + getTypeOutputInfo(typid, &typoutputfunc, &typIsVarlena); + fmgr_info(typoutputfunc, &typoutputfinfo); + prev_type = typid; + } + /* * At this point, we should see the main conversion specifier. Whether * or not an argument position was present, it's known that at least * one character remains in the string at this point. */ - value = PG_GETARG_DATUM(arg); - isNull = PG_ARGISNULL(arg); - typid = get_fn_expr_argtype(fcinfo->flinfo, arg); - switch (*cp) { case 's': case 'I': case 'L': - text_format_string_conversion(&str, *cp, typid, value, isNull); + text_format_string_conversion(&str, *cp, &typoutputfinfo, + value, isNull); break; default: ereport(ERROR, @@ -4091,6 +4231,12 @@ text_format(PG_FUNCTION_ARGS) } } + /* Don't need deconstruct_array results anymore. */ + if (elements != NULL) + pfree(elements); + if (nulls != NULL) + pfree(nulls); + /* Generate results. */ result = cstring_to_text_with_len(str.data, str.len); pfree(str.data); @@ -4099,12 +4245,11 @@ text_format(PG_FUNCTION_ARGS) } /* Format a %s, %I, or %L conversion. */ -void +static void text_format_string_conversion(StringInfo buf, char conversion, - Oid typid, Datum value, bool isNull) + FmgrInfo *typOutputInfo, + Datum value, bool isNull) { - Oid typOutput; - bool typIsVarlena; char *str; /* Handle NULL arguments before trying to stringify the value. */ @@ -4120,8 +4265,7 @@ text_format_string_conversion(StringInfo buf, char conversion, } /* Stringify. */ - getTypeOutputInfo(typid, &typOutput, &typIsVarlena); - str = OidOutputFunctionCall(typOutput, value); + str = OutputFunctionCall(typOutputInfo, value); /* Escape. */ if (conversion == 'I') diff --git a/src/test/regress/expected/text.out b/src/test/regress/expected/text.out index e45714f77e..b7565830d6 100644 --- a/src/test/regress/expected/text.out +++ b/src/test/regress/expected/text.out @@ -136,6 +136,40 @@ select quote_literal(e'\\'); E'\\' (1 row) +-- check variadic labeled argument +select concat(variadic array[1,2,3]); + concat +-------- + 123 +(1 row) + +select concat_ws(',', variadic array[1,2,3]); + concat_ws +----------- + 1,2,3 +(1 row) + +select concat_ws(',', variadic NULL); + concat_ws +----------- + +(1 row) + +select concat(variadic NULL) is NULL; + ?column? +---------- + t +(1 row) + +select concat(variadic '{}'::int[]) = ''; + ?column? +---------- + t +(1 row) + +--should fail +select concat_ws(',', variadic 10); +ERROR: VARIADIC argument must be an array /* * format */ @@ -228,7 +262,7 @@ select format('%1$', 1); ERROR: unterminated conversion specifier select format('%1$1', 1); ERROR: unrecognized conversion specifier "1" ---checkk mix of positional and ordered placeholders +-- check mix of positional and ordered placeholders select format('Hello %s %1$s %s', 'World', 'Hello again'); format ------------------------------- @@ -241,3 +275,56 @@ select format('Hello %s %s, %2$s %2$s', 'World', 'Hello again'); Hello World Hello again, Hello again Hello again (1 row) +-- check variadic labeled arguments +select format('%s, %s', variadic array['Hello','World']); + format +-------------- + Hello, World +(1 row) + +select format('%s, %s', variadic array[1, 2]); + format +-------- + 1, 2 +(1 row) + +select format('%s, %s', variadic array[true, false]); + format +-------- + t, f +(1 row) + +select format('%s, %s', variadic array[true, false]::text[]); + format +------------- + true, false +(1 row) + +-- check variadic with positional placeholders +select format('%2$s, %1$s', variadic array['first', 'second']); + format +--------------- + second, first +(1 row) + +select format('%2$s, %1$s', variadic array[1, 2]); + format +-------- + 2, 1 +(1 row) + +-- variadic argument can be NULL, but should not be referenced +select format('Hello', variadic NULL); + format +-------- + Hello +(1 row) + +-- variadic argument allows simulating more than FUNC_MAX_ARGS parameters +select format(string_agg('%s',','), variadic array_agg(i)) +from generate_series(1,200) g(i); + format +--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- + 1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32,33,34,35,36,37,38,39,40,41,42,43,44,45,46,47,48,49,50,51,52,53,54,55,56,57,58,59,60,61,62,63,64,65,66,67,68,69,70,71,72,73,74,75,76,77,78,79,80,81,82,83,84,85,86,87,88,89,90,91,92,93,94,95,96,97,98,99,100,101,102,103,104,105,106,107,108,109,110,111,112,113,114,115,116,117,118,119,120,121,122,123,124,125,126,127,128,129,130,131,132,133,134,135,136,137,138,139,140,141,142,143,144,145,146,147,148,149,150,151,152,153,154,155,156,157,158,159,160,161,162,163,164,165,166,167,168,169,170,171,172,173,174,175,176,177,178,179,180,181,182,183,184,185,186,187,188,189,190,191,192,193,194,195,196,197,198,199,200 +(1 row) + diff --git a/src/test/regress/sql/text.sql b/src/test/regress/sql/text.sql index 96e425d3cf..a96e9f7d1e 100644 --- a/src/test/regress/sql/text.sql +++ b/src/test/regress/sql/text.sql @@ -44,6 +44,14 @@ select i, left('ahoj', i), right('ahoj', i) from generate_series(-5, 5) t(i) ord select quote_literal(''); select quote_literal('abc'''); select quote_literal(e'\\'); +-- check variadic labeled argument +select concat(variadic array[1,2,3]); +select concat_ws(',', variadic array[1,2,3]); +select concat_ws(',', variadic NULL); +select concat(variadic NULL) is NULL; +select concat(variadic '{}'::int[]) = ''; +--should fail +select concat_ws(',', variadic 10); /* * format @@ -73,6 +81,19 @@ select format('%1$s %13$s', 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12); select format('%1s', 1); select format('%1$', 1); select format('%1$1', 1); ---checkk mix of positional and ordered placeholders +-- check mix of positional and ordered placeholders select format('Hello %s %1$s %s', 'World', 'Hello again'); select format('Hello %s %s, %2$s %2$s', 'World', 'Hello again'); +-- check variadic labeled arguments +select format('%s, %s', variadic array['Hello','World']); +select format('%s, %s', variadic array[1, 2]); +select format('%s, %s', variadic array[true, false]); +select format('%s, %s', variadic array[true, false]::text[]); +-- check variadic with positional placeholders +select format('%2$s, %1$s', variadic array['first', 'second']); +select format('%2$s, %1$s', variadic array[1, 2]); +-- variadic argument can be NULL, but should not be referenced +select format('Hello', variadic NULL); +-- variadic argument allows simulating more than FUNC_MAX_ARGS parameters +select format(string_agg('%s',','), variadic array_agg(i)) +from generate_series(1,200) g(i); -- 2.40.0