diff --git a/src/backend/utils/adt/orderedsetaggs.c b/src/backend/utils/adt/orderedsetaggs.c index 46b2694d89e..32002dca684 100644 --- a/src/backend/utils/adt/orderedsetaggs.c +++ b/src/backend/utils/adt/orderedsetaggs.c @@ -1516,6 +1516,17 @@ gp_percentile_cont_transition(FunctionCallInfo fcinfo, int64 first_row; int64 second_row; + /* + * Note: 'proargtypes' for this function in pg_proc.dat has 4 arguments. + * There are actually 5 arguments coming in here - the result of the + * previous call and 4 main arguments. + */ + if (PG_NARGS() != 5) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("wrong number of arguments to gp_percentile_cont_transition()"), + errhint("expected 5, got %d", PG_NARGS()))); + /* Return state for NULL inputs of val*/ if (PG_ARGISNULL(1) && !PG_ARGISNULL(0)) PG_RETURN_DATUM(PG_GETARG_DATUM(0)); @@ -1562,6 +1573,17 @@ gp_percentile_cont_transition(FunctionCallInfo fcinfo, { return_state = lerpfunc(prev_state, val, proportion); } + else if (PG_ARGISNULL(0)) + { + /* + * Neither of the rows we are after landed in this peer group, so we + * hand the previous state back unchanged. When that state is NULL + * the isnull flag has to travel with it: returning a bare Datum(0) + * as non-NULL gives wrong answers for by-value types and a NULL + * pointer dereference for by-reference ones. + */ + fcinfo->isnull = true; + } *cnt = *cnt + peer_count; if(*cnt > total_rows) @@ -1619,6 +1641,17 @@ gp_percentile_disc_transition(PG_FUNCTION_ARGS) { int64 rownum; + /* + * Note: 'proargtypes' for this function in pg_proc.dat has 4 arguments. + * There are actually 5 arguments coming in here - the result of the + * previous call and 4 main arguments. + */ + if (PG_NARGS() != 5) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("wrong number of arguments to gp_percentile_disc_transition()"), + errhint("expected 5, got %d", PG_NARGS()))); + /* Return state for NULL inputs of val*/ if (PG_ARGISNULL(1) && !PG_ARGISNULL(0)) PG_RETURN_DATUM(PG_GETARG_DATUM(0)); @@ -1634,7 +1667,6 @@ gp_percentile_disc_transition(PG_FUNCTION_ARGS) errmsg("percentile value %g is not between 0 and 1", percentile))); Datum prev_state = PG_GETARG_DATUM(0); - bool prev_state_isnull = PG_ARGISNULL(0); Datum val = PG_GETARG_DATUM(1); Datum return_state = prev_state; int64 total_rows = PG_GETARG_INT64(3); @@ -1659,6 +1691,11 @@ gp_percentile_disc_transition(PG_FUNCTION_ARGS) { return_state = val; } + else if (PG_ARGISNULL(0)) + { + /* see gp_percentile_cont_transition() */ + fcinfo->isnull = true; + } *cnt = *cnt + peer_count; @@ -1669,10 +1706,6 @@ gp_percentile_disc_transition(PG_FUNCTION_ARGS) fcinfo->flinfo->fn_extra = NULL; } - if (return_state == prev_state) { - fcinfo->isnull = prev_state_isnull; - } - PG_RETURN_DATUM(return_state); } diff --git a/src/test/regress/expected/percentile.out b/src/test/regress/expected/percentile.out index c1f4ec69da1..563a4864995 100644 --- a/src/test/regress/expected/percentile.out +++ b/src/test/regress/expected/percentile.out @@ -876,6 +876,100 @@ group by d1, d2; 55 | 1 (1 row) +-- +-- gp_percentile_cont()/gp_percentile_disc() are the split ordered-set +-- aggregates that ORCA rewrites percentile_cont()/percentile_disc()/median() +-- into. Their transition functions carry the running state on top of the +-- four aggregate arguments, so they read five arguments, while pg_proc.dat +-- describes four. That is left alone here so as not to force an initdb on a +-- stable branch; instead a direct call, which would read past the end of the +-- argument array, is rejected. +-- +select gp_percentile_cont_float8_transition(NULL::float8, 1, 1, 1); +ERROR: wrong number of arguments to gp_percentile_cont_transition() +HINT: expected 5, got 4 +select gp_percentile_cont_interval_transition(NULL::interval, 1, 1, 1); +ERROR: wrong number of arguments to gp_percentile_cont_transition() +HINT: expected 5, got 4 +select gp_percentile_cont_timestamp_transition(NULL::timestamp, 1, 1, 1); +ERROR: wrong number of arguments to gp_percentile_cont_transition() +HINT: expected 5, got 4 +select gp_percentile_cont_timestamptz_transition(NULL::timestamptz, 1, 1, 1); +ERROR: wrong number of arguments to gp_percentile_cont_transition() +HINT: expected 5, got 4 +select gp_percentile_disc_transition(NULL::numeric, 1, 1, 1); +ERROR: wrong number of arguments to gp_percentile_disc_transition() +HINT: expected 5, got 4 +-- On an empty input set the transition state stays NULL. The transition +-- functions hand that state back untouched and have to keep its isnull flag +-- with it: a bare Datum(0) escaping here reads as a bogus value for by-value +-- types and dereferences a NULL pointer for by-reference ones. +select gp_percentile_cont(0::float8, 0, 0, 0); + gp_percentile_cont +-------------------- + +(1 row) + +select gp_percentile_cont('0 hour'::interval, 0, 0, 0); + gp_percentile_cont +-------------------- + +(1 row) + +select gp_percentile_cont('2006-01-01 13:10:13'::timestamp, 0, 0, 0); + gp_percentile_cont +-------------------- + +(1 row) + +select gp_percentile_cont('2006-01-01 13:10:13+00'::timestamptz, 0, 0, 0); + gp_percentile_cont +-------------------- + +(1 row) + +select gp_percentile_disc(0::numeric, 0, 0, 0); + gp_percentile_disc +-------------------- + +(1 row) + +-- A value that really was picked has to come back, even when its Datum +-- representation happens to be 0. +select gp_percentile_disc(0::float8, 0, 1, 1); + gp_percentile_disc +-------------------- + 0 +(1 row) + +select gp_percentile_disc(0::int, 0, 1, 1); + gp_percentile_disc +-------------------- + 0 +(1 row) + +select gp_percentile_cont(0::float8, 0, 1, 1); + gp_percentile_cont +-------------------- + 0 +(1 row) + +-- The same, end to end: the smallest value of b is 0. +create table perczero (a int, b float8) distributed by (a); +insert into perczero select i, (i - 1)::float8 from generate_series(1, 10) i; +select percentile_disc(0) within group (order by b) from perczero; + percentile_disc +----------------- + 0 +(1 row) + +select percentile_cont(0) within group (order by b) from perczero; + percentile_cont +----------------- + 0 +(1 row) + +drop table perczero; drop view percv2; drop view percv; drop table perct; diff --git a/src/test/regress/sql/percentile.sql b/src/test/regress/sql/percentile.sql index bc7c327e770..dde74fc29f5 100644 --- a/src/test/regress/sql/percentile.sql +++ b/src/test/regress/sql/percentile.sql @@ -216,6 +216,40 @@ from mpp_22413 where d2 ='55' group by d1, d2; +-- +-- gp_percentile_cont()/gp_percentile_disc() are the split ordered-set +-- aggregates that ORCA rewrites percentile_cont()/percentile_disc()/median() +-- into. Their transition functions carry the running state on top of the +-- four aggregate arguments, so they read five arguments, while pg_proc.dat +-- describes four. That is left alone here so as not to force an initdb on a +-- stable branch; instead a direct call, which would read past the end of the +-- argument array, is rejected. +-- +select gp_percentile_cont_float8_transition(NULL::float8, 1, 1, 1); +select gp_percentile_cont_interval_transition(NULL::interval, 1, 1, 1); +select gp_percentile_cont_timestamp_transition(NULL::timestamp, 1, 1, 1); +select gp_percentile_cont_timestamptz_transition(NULL::timestamptz, 1, 1, 1); +select gp_percentile_disc_transition(NULL::numeric, 1, 1, 1); +-- On an empty input set the transition state stays NULL. The transition +-- functions hand that state back untouched and have to keep its isnull flag +-- with it: a bare Datum(0) escaping here reads as a bogus value for by-value +-- types and dereferences a NULL pointer for by-reference ones. +select gp_percentile_cont(0::float8, 0, 0, 0); +select gp_percentile_cont('0 hour'::interval, 0, 0, 0); +select gp_percentile_cont('2006-01-01 13:10:13'::timestamp, 0, 0, 0); +select gp_percentile_cont('2006-01-01 13:10:13+00'::timestamptz, 0, 0, 0); +select gp_percentile_disc(0::numeric, 0, 0, 0); +-- A value that really was picked has to come back, even when its Datum +-- representation happens to be 0. +select gp_percentile_disc(0::float8, 0, 1, 1); +select gp_percentile_disc(0::int, 0, 1, 1); +select gp_percentile_cont(0::float8, 0, 1, 1); +-- The same, end to end: the smallest value of b is 0. +create table perczero (a int, b float8) distributed by (a); +insert into perczero select i, (i - 1)::float8 from generate_series(1, 10) i; +select percentile_disc(0) within group (order by b) from perczero; +select percentile_cont(0) within group (order by b) from perczero; +drop table perczero; drop view percv2; drop view percv; drop table perct;