range_agg with multirange inputs
Here is a patch adding range_agg(anymultirange). Previously range_agg
only accepted anyrange.
Here is a bug report from last month requesting this addition:
/messages/by-id/CAOC8YUcOtAGscPa31ik8UEMzgn8uAWA09s6CYOGPyP9_cBbWTw@mail.gmail.com
As that message points out, range_intersect_agg accepts either anyrange
or anymultirange, so it makes sense for range_agg to do the same.
I noticed that the docs only mentioned range_intersect_agg(anyrange), so
I added the anymultirange versions of both on the aggregate functions page.
I also added a few more tests for range_intersect_agg since the coverage
there seemed light.
Yours,
--
Paul ~{:-)
pj@illuminatedcomputing.com
Attachments:
v1-0001-Add-range_agg-with-multirange-inputs.patchtext/x-patch; charset=UTF-8; name=v1-0001-Add-range_agg-with-multirange-inputs.patchDownload
From 116c9dd5b3fbb6626d81588c124cf8fdcb4185ce Mon Sep 17 00:00:00 2001
From: "Paul A. Jungwirth" <pj@illuminatedcomputing.com>
Date: Fri, 10 Dec 2021 16:04:57 -0800
Subject: [PATCH v1] Add range_agg with multirange inputs
---
doc/src/sgml/func.sgml | 30 ++++++
src/backend/utils/adt/multirangetypes.c | 60 +++++++++++
src/include/catalog/pg_aggregate.dat | 3 +
src/include/catalog/pg_proc.dat | 11 ++
src/test/regress/expected/multirangetypes.out | 100 ++++++++++++++++++
src/test/regress/expected/opr_sanity.out | 1 +
src/test/regress/sql/multirangetypes.sql | 21 ++++
src/test/regress/sql/opr_sanity.sql | 1 +
8 files changed, 227 insertions(+)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 0a725a6711..697bf1924e 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -19951,6 +19951,21 @@ SELECT NULLIF(value, '(none)') ...
<entry>No</entry>
</row>
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>range_agg</primary>
+ </indexterm>
+ <function>range_agg</function> ( <parameter>value</parameter>
+ <type>anymultirange</type> )
+ <returnvalue>anymultirange</returnvalue>
+ </para>
+ <para>
+ Computes the union of the non-null input values.
+ </para></entry>
+ <entry>No</entry>
+ </row>
+
<row>
<entry role="func_table_entry"><para role="func_signature">
<indexterm>
@@ -19966,6 +19981,21 @@ SELECT NULLIF(value, '(none)') ...
<entry>No</entry>
</row>
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>range_intersect_agg</primary>
+ </indexterm>
+ <function>range_intersect_agg</function> ( <parameter>value</parameter>
+ <type>anymultirange</type> )
+ <returnvalue>anymultirange</returnvalue>
+ </para>
+ <para>
+ Computes the intersection of the non-null input values.
+ </para></entry>
+ <entry>No</entry>
+ </row>
+
<row>
<entry role="func_table_entry"><para role="func_signature">
<indexterm>
diff --git a/src/backend/utils/adt/multirangetypes.c b/src/backend/utils/adt/multirangetypes.c
index 7773215564..beaa843eab 100644
--- a/src/backend/utils/adt/multirangetypes.c
+++ b/src/backend/utils/adt/multirangetypes.c
@@ -1394,6 +1394,66 @@ range_agg_finalfn(PG_FUNCTION_ARGS)
PG_RETURN_MULTIRANGE_P(make_multirange(mltrngtypoid, typcache->rngtype, range_count, ranges));
}
+/*
+ * multirange_agg_transfn: combine adjacent/overlapping multiranges.
+ *
+ * All we do here is gather the input multiranges' ranges into an array
+ * so that the finalfn can sort and combine them.
+ */
+Datum
+multirange_agg_transfn(PG_FUNCTION_ARGS)
+{
+ MemoryContext aggContext;
+ Oid mltrngtypoid;
+ TypeCacheEntry *typcache;
+ TypeCacheEntry *rngtypcache;
+ ArrayBuildState *state;
+ MultirangeType *current;
+ int32 range_count;
+ RangeType **ranges;
+ int32 i;
+
+ if (!AggCheckCallContext(fcinfo, &aggContext))
+ elog(ERROR, "multirange_agg_transfn called in non-aggregate context");
+
+ mltrngtypoid = get_fn_expr_argtype(fcinfo->flinfo, 1);
+ if (!type_is_multirange(mltrngtypoid))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("range_agg must be called with a multirange")));
+
+ typcache = multirange_get_typcache(fcinfo, mltrngtypoid);
+ rngtypcache = typcache->rngtype;
+
+ if (PG_ARGISNULL(0))
+ state = initArrayResult(rngtypcache->type_id, aggContext, false);
+ else
+ state = (ArrayBuildState *) PG_GETARG_POINTER(0);
+
+ /* skip NULLs */
+ if (!PG_ARGISNULL(1))
+ {
+ current = PG_GETARG_MULTIRANGE_P(1);
+ multirange_deserialize(rngtypcache, current, &range_count, &ranges);
+ if (range_count == 0)
+ {
+ /*
+ * Add an empty range so we get an empty result (not a null result).
+ */
+ accumArrayResult(state,
+ RangeTypePGetDatum(make_empty_range(rngtypcache)),
+ false, rngtypcache->type_id, aggContext);
+ }
+ else
+ {
+ for (i = 0; i < range_count; i++)
+ accumArrayResult(state, RangeTypePGetDatum(ranges[i]), false, rngtypcache->type_id, aggContext);
+ }
+ }
+
+ PG_RETURN_POINTER(state);
+}
+
Datum
multirange_intersect_agg_transfn(PG_FUNCTION_ARGS)
{
diff --git a/src/include/catalog/pg_aggregate.dat b/src/include/catalog/pg_aggregate.dat
index fc6d3bfd94..5a06583798 100644
--- a/src/include/catalog/pg_aggregate.dat
+++ b/src/include/catalog/pg_aggregate.dat
@@ -557,6 +557,9 @@
{ aggfnoid => 'range_agg(anyrange)', aggtransfn => 'range_agg_transfn',
aggfinalfn => 'range_agg_finalfn', aggfinalextra => 't',
aggtranstype => 'internal' },
+{ aggfnoid => 'range_agg(anymultirange)', aggtransfn => 'multirange_agg_transfn',
+ aggfinalfn => 'multirange_agg_finalfn', aggfinalextra => 't',
+ aggtranstype => 'internal' },
# json
{ aggfnoid => 'json_agg', aggtransfn => 'json_agg_transfn',
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 79d787cd26..01c5c52729 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -10619,6 +10619,17 @@
proname => 'range_agg', prokind => 'a', proisstrict => 'f',
prorettype => 'anymultirange', proargtypes => 'anyrange',
prosrc => 'aggregate_dummy' },
+{ oid => '8000', descr => 'aggregate transition function',
+ proname => 'multirange_agg_transfn', proisstrict => 'f', prorettype => 'internal',
+ proargtypes => 'internal anymultirange', prosrc => 'multirange_agg_transfn' },
+{ oid => '8001', descr => 'combine aggregate input into a multirange',
+ proname => 'range_agg', prokind => 'a', proisstrict => 'f',
+ prorettype => 'anymultirange', proargtypes => 'anymultirange',
+ prosrc => 'aggregate_dummy' },
+{ oid => '8002', descr => 'aggregate final function',
+ proname => 'multirange_agg_finalfn', proisstrict => 'f',
+ prorettype => 'anymultirange', proargtypes => 'internal anymultirange',
+ prosrc => 'range_agg_finalfn' },
{ oid => '4388', descr => 'range aggregate by intersecting',
proname => 'multirange_intersect_agg_transfn', prorettype => 'anymultirange',
proargtypes => 'anymultirange anymultirange',
diff --git a/src/test/regress/expected/multirangetypes.out b/src/test/regress/expected/multirangetypes.out
index f5f089741c..7c91e3c5f2 100644
--- a/src/test/regress/expected/multirangetypes.out
+++ b/src/test/regress/expected/multirangetypes.out
@@ -2771,6 +2771,70 @@ FROM (VALUES
{[a,f],[g,j)}
(1 row)
+-- range_agg with multirange inputs
+select range_agg(nmr) from nummultirange_test;
+ range_agg
+-----------
+ {(,)}
+(1 row)
+
+select range_agg(nmr) from nummultirange_test where false;
+ range_agg
+-----------
+
+(1 row)
+
+select range_agg(null::nummultirange) from nummultirange_test;
+ range_agg
+-----------
+
+(1 row)
+
+select range_agg(nmr) from (values ('{}'::nummultirange)) t(nmr);
+ range_agg
+-----------
+ {}
+(1 row)
+
+select range_agg(nmr) from (values ('{}'::nummultirange), ('{}'::nummultirange)) t(nmr);
+ range_agg
+-----------
+ {}
+(1 row)
+
+select range_agg(nmr) from (values ('{[1,2]}'::nummultirange)) t(nmr);
+ range_agg
+-----------
+ {[1,2]}
+(1 row)
+
+select range_agg(nmr) from (values ('{[1,2], [5,6]}'::nummultirange)) t(nmr);
+ range_agg
+---------------
+ {[1,2],[5,6]}
+(1 row)
+
+select range_agg(nmr) from (values ('{[1,2], [2,3]}'::nummultirange)) t(nmr);
+ range_agg
+-----------
+ {[1,3]}
+(1 row)
+
+select range_agg(nmr) from (values ('{[1,2]}'::nummultirange), ('{[5,6]}'::nummultirange)) t(nmr);
+ range_agg
+---------------
+ {[1,2],[5,6]}
+(1 row)
+
+select range_agg(nmr) from (values ('{[1,2]}'::nummultirange), ('{[2,3]}'::nummultirange)) t(nmr);
+ range_agg
+-----------
+ {[1,3]}
+(1 row)
+
+--
+-- range_intersect_agg function
+--
select range_intersect_agg(nmr) from nummultirange_test;
range_intersect_agg
---------------------
@@ -2783,13 +2847,49 @@ select range_intersect_agg(nmr) from nummultirange_test where false;
(1 row)
+select range_intersect_agg(null::nummultirange) from nummultirange_test;
+ range_intersect_agg
+---------------------
+
+(1 row)
+
+select range_intersect_agg(nmr) from (values ('{[1,3]}'::nummultirange), ('{[6,12]}'::nummultirange)) t(nmr);
+ range_intersect_agg
+---------------------
+ {}
+(1 row)
+
+select range_intersect_agg(nmr) from (values ('{[1,6]}'::nummultirange), ('{[3,12]}'::nummultirange)) t(nmr);
+ range_intersect_agg
+---------------------
+ {[3,6]}
+(1 row)
+
+select range_intersect_agg(nmr) from (values ('{[1,6], [10,12]}'::nummultirange), ('{[4,14]}'::nummultirange)) t(nmr);
+ range_intersect_agg
+---------------------
+ {[4,6],[10,12]}
+(1 row)
+
-- test with just one input:
+select range_intersect_agg(nmr) from (values ('{}'::nummultirange)) t(nmr);
+ range_intersect_agg
+---------------------
+ {}
+(1 row)
+
select range_intersect_agg(nmr) from (values ('{[1,2]}'::nummultirange)) t(nmr);
range_intersect_agg
---------------------
{[1,2]}
(1 row)
+select range_intersect_agg(nmr) from (values ('{[1,6], [10,12]}'::nummultirange)) t(nmr);
+ range_intersect_agg
+---------------------
+ {[1,6],[10,12]}
+(1 row)
+
select range_intersect_agg(nmr) from nummultirange_test where nmr @> 4.0;
range_intersect_agg
---------------------
diff --git a/src/test/regress/expected/opr_sanity.out b/src/test/regress/expected/opr_sanity.out
index 562b586d8e..0561d2f705 100644
--- a/src/test/regress/expected/opr_sanity.out
+++ b/src/test/regress/expected/opr_sanity.out
@@ -193,6 +193,7 @@ WHERE p1.oid != p2.oid AND
p2.prosrc NOT LIKE E'range\\_constructor_' AND
p1.prosrc NOT LIKE E'multirange\\_constructor_' AND
p2.prosrc NOT LIKE E'multirange\\_constructor_' AND
+ p2.prosrc NOT LIKE E'range\\_agg\\_finalfn' AND
(p1.proargtypes[1] < p2.proargtypes[1])
ORDER BY 1, 2;
proargtypes | proargtypes
diff --git a/src/test/regress/sql/multirangetypes.sql b/src/test/regress/sql/multirangetypes.sql
index ad50afc0f5..807a3a9b98 100644
--- a/src/test/regress/sql/multirangetypes.sql
+++ b/src/test/regress/sql/multirangetypes.sql
@@ -570,10 +570,31 @@ FROM (VALUES
('[h,j)'::textrange)
) t(r);
+-- range_agg with multirange inputs
+select range_agg(nmr) from nummultirange_test;
+select range_agg(nmr) from nummultirange_test where false;
+select range_agg(null::nummultirange) from nummultirange_test;
+select range_agg(nmr) from (values ('{}'::nummultirange)) t(nmr);
+select range_agg(nmr) from (values ('{}'::nummultirange), ('{}'::nummultirange)) t(nmr);
+select range_agg(nmr) from (values ('{[1,2]}'::nummultirange)) t(nmr);
+select range_agg(nmr) from (values ('{[1,2], [5,6]}'::nummultirange)) t(nmr);
+select range_agg(nmr) from (values ('{[1,2], [2,3]}'::nummultirange)) t(nmr);
+select range_agg(nmr) from (values ('{[1,2]}'::nummultirange), ('{[5,6]}'::nummultirange)) t(nmr);
+select range_agg(nmr) from (values ('{[1,2]}'::nummultirange), ('{[2,3]}'::nummultirange)) t(nmr);
+
+--
+-- range_intersect_agg function
+--
select range_intersect_agg(nmr) from nummultirange_test;
select range_intersect_agg(nmr) from nummultirange_test where false;
+select range_intersect_agg(null::nummultirange) from nummultirange_test;
+select range_intersect_agg(nmr) from (values ('{[1,3]}'::nummultirange), ('{[6,12]}'::nummultirange)) t(nmr);
+select range_intersect_agg(nmr) from (values ('{[1,6]}'::nummultirange), ('{[3,12]}'::nummultirange)) t(nmr);
+select range_intersect_agg(nmr) from (values ('{[1,6], [10,12]}'::nummultirange), ('{[4,14]}'::nummultirange)) t(nmr);
-- test with just one input:
+select range_intersect_agg(nmr) from (values ('{}'::nummultirange)) t(nmr);
select range_intersect_agg(nmr) from (values ('{[1,2]}'::nummultirange)) t(nmr);
+select range_intersect_agg(nmr) from (values ('{[1,6], [10,12]}'::nummultirange)) t(nmr);
select range_intersect_agg(nmr) from nummultirange_test where nmr @> 4.0;
create table nummultirange_test2(nmr nummultirange);
diff --git a/src/test/regress/sql/opr_sanity.sql b/src/test/regress/sql/opr_sanity.sql
index 5a9c479692..bb129c42ff 100644
--- a/src/test/regress/sql/opr_sanity.sql
+++ b/src/test/regress/sql/opr_sanity.sql
@@ -153,6 +153,7 @@ WHERE p1.oid != p2.oid AND
p2.prosrc NOT LIKE E'range\\_constructor_' AND
p1.prosrc NOT LIKE E'multirange\\_constructor_' AND
p2.prosrc NOT LIKE E'multirange\\_constructor_' AND
+ p2.prosrc NOT LIKE E'range\\_agg\\_finalfn' AND
(p1.proargtypes[1] < p2.proargtypes[1])
ORDER BY 1, 2;
--
2.25.1
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested
This applies (with some fuzz) and passes installcheck-world, but a rebase
is needed, because 3 lines of context aren't enough to get the doc changes
in the right place in the aggregate function table. (I think generating
the patch with 4 lines of context would be enough to keep that from being
a recurring issue.)
One thing that seems a bit funny is this message in the new
multirange_agg_transfn:
+ if (!type_is_multirange(mltrngtypoid))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("range_agg must be called with a multirange")));It's clearly copied from the corresponding test and message in
range_agg_transfn. They both say "range_agg must be called ...", which
makes perfect sense, as from the user's perspective both messages come
from (different overloads of) a function named range_agg.
Still, it could be odd to have (again from the user's perspective)
a function named range_agg that sometimes says "range_agg must be
called with a range" and other times says "range_agg must be called
with a multirange".
I'm not sure how to tweak the wording (of either message or both) to
make that less weird, but there's probably a way.
I kind of wonder whether either message is really reachable, at least
through the aggregate machinery in the expected way. Won't that machinery
ensure that it is calling the right transfn with the right type of
argument? If that's the case, maybe the messages could only be seen
by someone calling the transfn directly ... which also seems ruled out
for these transfns because the state type is internal. Is this whole test
more of the nature of an assertion?
Regards,
-Chap
The new status of this patch is: Waiting on Author
On 2/26/22 17:13, Chapman Flack wrote:
This applies (with some fuzz) and passes installcheck-world, but a rebase
is needed, because 3 lines of context aren't enough to get the doc changes
in the right place in the aggregate function table. (I think generating
the patch with 4 lines of context would be enough to keep that from being
a recurring issue.)
Thank you for the review and the tip re 4 lines of context! Rebase attached.
One thing that seems a bit funny is this message in the new
multirange_agg_transfn:+ if (!type_is_multirange(mltrngtypoid)) + ereport(ERROR, + (errcode(ERRCODE_DATATYPE_MISMATCH), + errmsg("range_agg must be called with a multirange")));
I agree it would be more helpful to users to let them know we can take
either kind of argument. I changed the message to "range_agg must be
called with a range or multirange". How does that seem?
I kind of wonder whether either message is really reachable, at least
through the aggregate machinery in the expected way. Won't that machinery
ensure that it is calling the right transfn with the right type of
argument? If that's the case, maybe the messages could only be seen
by someone calling the transfn directly ... which also seems ruled out
for these transfns because the state type is internal. Is this whole test
more of the nature of an assertion?
I don't think they are reachable, so perhaps they are more like asserts.
Do you think I should change it? It seems like a worthwhile check in any
case.
Yours,
--
Paul ~{:-)
pj@illuminatedcomputing.com
Attachments:
v2-0001-Add-range_agg-with-multirange-inputs.patchtext/x-patch; charset=UTF-8; name=v2-0001-Add-range_agg-with-multirange-inputs.patchDownload
From a6689485aab9b1aaa6e866f2a577368c7a0e324e Mon Sep 17 00:00:00 2001
From: "Paul A. Jungwirth" <pj@illuminatedcomputing.com>
Date: Fri, 10 Dec 2021 16:04:57 -0800
Subject: [PATCH v2] Add range_agg with multirange inputs
---
doc/src/sgml/func.sgml | 30 ++++++
src/backend/utils/adt/multirangetypes.c | 62 ++++++++++-
src/include/catalog/pg_aggregate.dat | 3 +
src/include/catalog/pg_proc.dat | 11 ++
src/test/regress/expected/multirangetypes.out | 100 ++++++++++++++++++
src/test/regress/expected/opr_sanity.out | 1 +
src/test/regress/sql/multirangetypes.sql | 21 ++++
src/test/regress/sql/opr_sanity.sql | 1 +
8 files changed, 228 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index df3cd5987b..6a72785327 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -19959,8 +19959,23 @@ SELECT NULLIF(value, '(none)') ...
</para></entry>
<entry>No</entry>
</row>
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>range_agg</primary>
+ </indexterm>
+ <function>range_agg</function> ( <parameter>value</parameter>
+ <type>anymultirange</type> )
+ <returnvalue>anymultirange</returnvalue>
+ </para>
+ <para>
+ Computes the union of the non-null input values.
+ </para></entry>
+ <entry>No</entry>
+ </row>
+
<row>
<entry role="func_table_entry"><para role="func_signature">
<indexterm>
<primary>max</primary>
@@ -20012,8 +20027,23 @@ SELECT NULLIF(value, '(none)') ...
</para></entry>
<entry>No</entry>
</row>
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>range_intersect_agg</primary>
+ </indexterm>
+ <function>range_intersect_agg</function> ( <parameter>value</parameter>
+ <type>anymultirange</type> )
+ <returnvalue>anymultirange</returnvalue>
+ </para>
+ <para>
+ Computes the intersection of the non-null input values.
+ </para></entry>
+ <entry>No</entry>
+ </row>
+
<row>
<entry role="func_table_entry"><para role="func_signature">
<indexterm>
<primary>range_intersect_agg</primary>
diff --git a/src/backend/utils/adt/multirangetypes.c b/src/backend/utils/adt/multirangetypes.c
index c474b24431..0efef8cf35 100644
--- a/src/backend/utils/adt/multirangetypes.c
+++ b/src/backend/utils/adt/multirangetypes.c
@@ -1346,9 +1346,9 @@ range_agg_transfn(PG_FUNCTION_ARGS)
rngtypoid = get_fn_expr_argtype(fcinfo->flinfo, 1);
if (!type_is_range(rngtypoid))
ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("range_agg must be called with a range")));
+ errmsg("range_agg must be called with a range or multirange")));
if (PG_ARGISNULL(0))
state = initArrayResult(rngtypoid, aggContext, false);
else
@@ -1397,8 +1397,68 @@ range_agg_finalfn(PG_FUNCTION_ARGS)
PG_RETURN_MULTIRANGE_P(make_multirange(mltrngtypoid, typcache->rngtype, range_count, ranges));
}
+/*
+ * multirange_agg_transfn: combine adjacent/overlapping multiranges.
+ *
+ * All we do here is gather the input multiranges' ranges into an array
+ * so that the finalfn can sort and combine them.
+ */
+Datum
+multirange_agg_transfn(PG_FUNCTION_ARGS)
+{
+ MemoryContext aggContext;
+ Oid mltrngtypoid;
+ TypeCacheEntry *typcache;
+ TypeCacheEntry *rngtypcache;
+ ArrayBuildState *state;
+ MultirangeType *current;
+ int32 range_count;
+ RangeType **ranges;
+ int32 i;
+
+ if (!AggCheckCallContext(fcinfo, &aggContext))
+ elog(ERROR, "multirange_agg_transfn called in non-aggregate context");
+
+ mltrngtypoid = get_fn_expr_argtype(fcinfo->flinfo, 1);
+ if (!type_is_multirange(mltrngtypoid))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("range_agg must be called with a range or multirange")));
+
+ typcache = multirange_get_typcache(fcinfo, mltrngtypoid);
+ rngtypcache = typcache->rngtype;
+
+ if (PG_ARGISNULL(0))
+ state = initArrayResult(rngtypcache->type_id, aggContext, false);
+ else
+ state = (ArrayBuildState *) PG_GETARG_POINTER(0);
+
+ /* skip NULLs */
+ if (!PG_ARGISNULL(1))
+ {
+ current = PG_GETARG_MULTIRANGE_P(1);
+ multirange_deserialize(rngtypcache, current, &range_count, &ranges);
+ if (range_count == 0)
+ {
+ /*
+ * Add an empty range so we get an empty result (not a null result).
+ */
+ accumArrayResult(state,
+ RangeTypePGetDatum(make_empty_range(rngtypcache)),
+ false, rngtypcache->type_id, aggContext);
+ }
+ else
+ {
+ for (i = 0; i < range_count; i++)
+ accumArrayResult(state, RangeTypePGetDatum(ranges[i]), false, rngtypcache->type_id, aggContext);
+ }
+ }
+
+ PG_RETURN_POINTER(state);
+}
+
Datum
multirange_intersect_agg_transfn(PG_FUNCTION_ARGS)
{
MemoryContext aggContext;
diff --git a/src/include/catalog/pg_aggregate.dat b/src/include/catalog/pg_aggregate.dat
index 2843f4b415..15b4e6461a 100644
--- a/src/include/catalog/pg_aggregate.dat
+++ b/src/include/catalog/pg_aggregate.dat
@@ -562,8 +562,11 @@
aggtranstype => 'anymultirange' },
{ aggfnoid => 'range_agg(anyrange)', aggtransfn => 'range_agg_transfn',
aggfinalfn => 'range_agg_finalfn', aggfinalextra => 't',
aggtranstype => 'internal' },
+{ aggfnoid => 'range_agg(anymultirange)', aggtransfn => 'multirange_agg_transfn',
+ aggfinalfn => 'multirange_agg_finalfn', aggfinalextra => 't',
+ aggtranstype => 'internal' },
# json
{ aggfnoid => 'json_agg', aggtransfn => 'json_agg_transfn',
aggfinalfn => 'json_agg_finalfn', aggtranstype => 'internal' },
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index bf88858171..356fc7318b 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -10611,8 +10611,19 @@
{ oid => '4301', descr => 'combine aggregate input into a multirange',
proname => 'range_agg', prokind => 'a', proisstrict => 'f',
prorettype => 'anymultirange', proargtypes => 'anyrange',
prosrc => 'aggregate_dummy' },
+{ oid => '8000', descr => 'aggregate transition function',
+ proname => 'multirange_agg_transfn', proisstrict => 'f', prorettype => 'internal',
+ proargtypes => 'internal anymultirange', prosrc => 'multirange_agg_transfn' },
+{ oid => '8001', descr => 'combine aggregate input into a multirange',
+ proname => 'range_agg', prokind => 'a', proisstrict => 'f',
+ prorettype => 'anymultirange', proargtypes => 'anymultirange',
+ prosrc => 'aggregate_dummy' },
+{ oid => '8002', descr => 'aggregate final function',
+ proname => 'multirange_agg_finalfn', proisstrict => 'f',
+ prorettype => 'anymultirange', proargtypes => 'internal anymultirange',
+ prosrc => 'range_agg_finalfn' },
{ oid => '4388', descr => 'range aggregate by intersecting',
proname => 'multirange_intersect_agg_transfn', prorettype => 'anymultirange',
proargtypes => 'anymultirange anymultirange',
prosrc => 'multirange_intersect_agg_transfn' },
diff --git a/src/test/regress/expected/multirangetypes.out b/src/test/regress/expected/multirangetypes.out
index bde3f248a7..ac2eb84c3a 100644
--- a/src/test/regress/expected/multirangetypes.out
+++ b/src/test/regress/expected/multirangetypes.out
@@ -2783,8 +2783,72 @@ FROM (VALUES
---------------
{[a,f],[g,j)}
(1 row)
+-- range_agg with multirange inputs
+select range_agg(nmr) from nummultirange_test;
+ range_agg
+-----------
+ {(,)}
+(1 row)
+
+select range_agg(nmr) from nummultirange_test where false;
+ range_agg
+-----------
+
+(1 row)
+
+select range_agg(null::nummultirange) from nummultirange_test;
+ range_agg
+-----------
+
+(1 row)
+
+select range_agg(nmr) from (values ('{}'::nummultirange)) t(nmr);
+ range_agg
+-----------
+ {}
+(1 row)
+
+select range_agg(nmr) from (values ('{}'::nummultirange), ('{}'::nummultirange)) t(nmr);
+ range_agg
+-----------
+ {}
+(1 row)
+
+select range_agg(nmr) from (values ('{[1,2]}'::nummultirange)) t(nmr);
+ range_agg
+-----------
+ {[1,2]}
+(1 row)
+
+select range_agg(nmr) from (values ('{[1,2], [5,6]}'::nummultirange)) t(nmr);
+ range_agg
+---------------
+ {[1,2],[5,6]}
+(1 row)
+
+select range_agg(nmr) from (values ('{[1,2], [2,3]}'::nummultirange)) t(nmr);
+ range_agg
+-----------
+ {[1,3]}
+(1 row)
+
+select range_agg(nmr) from (values ('{[1,2]}'::nummultirange), ('{[5,6]}'::nummultirange)) t(nmr);
+ range_agg
+---------------
+ {[1,2],[5,6]}
+(1 row)
+
+select range_agg(nmr) from (values ('{[1,2]}'::nummultirange), ('{[2,3]}'::nummultirange)) t(nmr);
+ range_agg
+-----------
+ {[1,3]}
+(1 row)
+
+--
+-- range_intersect_agg function
+--
select range_intersect_agg(nmr) from nummultirange_test;
range_intersect_agg
---------------------
{}
@@ -2795,15 +2859,51 @@ select range_intersect_agg(nmr) from nummultirange_test where false;
---------------------
(1 row)
+select range_intersect_agg(null::nummultirange) from nummultirange_test;
+ range_intersect_agg
+---------------------
+
+(1 row)
+
+select range_intersect_agg(nmr) from (values ('{[1,3]}'::nummultirange), ('{[6,12]}'::nummultirange)) t(nmr);
+ range_intersect_agg
+---------------------
+ {}
+(1 row)
+
+select range_intersect_agg(nmr) from (values ('{[1,6]}'::nummultirange), ('{[3,12]}'::nummultirange)) t(nmr);
+ range_intersect_agg
+---------------------
+ {[3,6]}
+(1 row)
+
+select range_intersect_agg(nmr) from (values ('{[1,6], [10,12]}'::nummultirange), ('{[4,14]}'::nummultirange)) t(nmr);
+ range_intersect_agg
+---------------------
+ {[4,6],[10,12]}
+(1 row)
+
-- test with just one input:
+select range_intersect_agg(nmr) from (values ('{}'::nummultirange)) t(nmr);
+ range_intersect_agg
+---------------------
+ {}
+(1 row)
+
select range_intersect_agg(nmr) from (values ('{[1,2]}'::nummultirange)) t(nmr);
range_intersect_agg
---------------------
{[1,2]}
(1 row)
+select range_intersect_agg(nmr) from (values ('{[1,6], [10,12]}'::nummultirange)) t(nmr);
+ range_intersect_agg
+---------------------
+ {[1,6],[10,12]}
+(1 row)
+
select range_intersect_agg(nmr) from nummultirange_test where nmr @> 4.0;
range_intersect_agg
---------------------
{[3,5)}
diff --git a/src/test/regress/expected/opr_sanity.out b/src/test/regress/expected/opr_sanity.out
index 4ce6c039b4..046d2006d5 100644
--- a/src/test/regress/expected/opr_sanity.out
+++ b/src/test/regress/expected/opr_sanity.out
@@ -192,8 +192,9 @@ WHERE p1.oid != p2.oid AND
p1.prosrc NOT LIKE E'range\\_constructor_' AND
p2.prosrc NOT LIKE E'range\\_constructor_' AND
p1.prosrc NOT LIKE E'multirange\\_constructor_' AND
p2.prosrc NOT LIKE E'multirange\\_constructor_' AND
+ p2.prosrc NOT LIKE E'range\\_agg\\_finalfn' AND
(p1.proargtypes[1] < p2.proargtypes[1])
ORDER BY 1, 2;
proargtypes | proargtypes
-----------------------------+--------------------------
diff --git a/src/test/regress/sql/multirangetypes.sql b/src/test/regress/sql/multirangetypes.sql
index df1edb4d31..1abcaeddb5 100644
--- a/src/test/regress/sql/multirangetypes.sql
+++ b/src/test/regress/sql/multirangetypes.sql
@@ -571,12 +571,33 @@ FROM (VALUES
('[g,h)'::textrange),
('[h,j)'::textrange)
) t(r);
+-- range_agg with multirange inputs
+select range_agg(nmr) from nummultirange_test;
+select range_agg(nmr) from nummultirange_test where false;
+select range_agg(null::nummultirange) from nummultirange_test;
+select range_agg(nmr) from (values ('{}'::nummultirange)) t(nmr);
+select range_agg(nmr) from (values ('{}'::nummultirange), ('{}'::nummultirange)) t(nmr);
+select range_agg(nmr) from (values ('{[1,2]}'::nummultirange)) t(nmr);
+select range_agg(nmr) from (values ('{[1,2], [5,6]}'::nummultirange)) t(nmr);
+select range_agg(nmr) from (values ('{[1,2], [2,3]}'::nummultirange)) t(nmr);
+select range_agg(nmr) from (values ('{[1,2]}'::nummultirange), ('{[5,6]}'::nummultirange)) t(nmr);
+select range_agg(nmr) from (values ('{[1,2]}'::nummultirange), ('{[2,3]}'::nummultirange)) t(nmr);
+
+--
+-- range_intersect_agg function
+--
select range_intersect_agg(nmr) from nummultirange_test;
select range_intersect_agg(nmr) from nummultirange_test where false;
+select range_intersect_agg(null::nummultirange) from nummultirange_test;
+select range_intersect_agg(nmr) from (values ('{[1,3]}'::nummultirange), ('{[6,12]}'::nummultirange)) t(nmr);
+select range_intersect_agg(nmr) from (values ('{[1,6]}'::nummultirange), ('{[3,12]}'::nummultirange)) t(nmr);
+select range_intersect_agg(nmr) from (values ('{[1,6], [10,12]}'::nummultirange), ('{[4,14]}'::nummultirange)) t(nmr);
-- test with just one input:
+select range_intersect_agg(nmr) from (values ('{}'::nummultirange)) t(nmr);
select range_intersect_agg(nmr) from (values ('{[1,2]}'::nummultirange)) t(nmr);
+select range_intersect_agg(nmr) from (values ('{[1,6], [10,12]}'::nummultirange)) t(nmr);
select range_intersect_agg(nmr) from nummultirange_test where nmr @> 4.0;
create table nummultirange_test2(nmr nummultirange);
create index nummultirange_test2_hash_idx on nummultirange_test2 using hash (nmr);
diff --git a/src/test/regress/sql/opr_sanity.sql b/src/test/regress/sql/opr_sanity.sql
index 2b292851e3..bbcffda228 100644
--- a/src/test/regress/sql/opr_sanity.sql
+++ b/src/test/regress/sql/opr_sanity.sql
@@ -152,8 +152,9 @@ WHERE p1.oid != p2.oid AND
p1.prosrc NOT LIKE E'range\\_constructor_' AND
p2.prosrc NOT LIKE E'range\\_constructor_' AND
p1.prosrc NOT LIKE E'multirange\\_constructor_' AND
p2.prosrc NOT LIKE E'multirange\\_constructor_' AND
+ p2.prosrc NOT LIKE E'range\\_agg\\_finalfn' AND
(p1.proargtypes[1] < p2.proargtypes[1])
ORDER BY 1, 2;
SELECT DISTINCT p1.proargtypes[2]::regtype, p2.proargtypes[2]::regtype
--
2.25.1
On 02/28/22 23:31, Paul Jungwirth wrote:
On 2/26/22 17:13, Chapman Flack wrote:
(I think generating
the patch with 4 lines of context would be enough to keep that from being
a recurring issue.)Thank you for the review and the tip re 4 lines of context! Rebase attached.
I think the 4 lines should suffice, but it looks like this patch was
generated from a rebase of the old one (with three lines) that ended up
putting the new 'range_agg' entry ahead of 'max' in func.sgml, which
position is now baked into the 4 lines of context. :)
So I think it needs a bit of manual attention to get the additions back
in the right places, and then a 4-context-lines patch generated from that.
I changed the message to "range_agg must be called
with a range or multirange". How does that seem?
That works for me.
I kind of wonder whether either message is really reachable, at least
through the aggregate machinery in the expected way. Won't that machinery
ensure that it is calling the right transfn with the right type of
argument? If that's the case, maybe the messages could only be seen
by someone calling the transfn directly ... which also seems ruled out
for these transfns because the state type is internal. Is this whole test
more of the nature of an assertion?I don't think they are reachable, so perhaps they are more like asserts. Do
you think I should change it? It seems like a worthwhile check in any case.
I would not change them to actual Assert, which would blow up the whole
process on failure. If it's a genuine "not expected to happen" case,
maybe changing it to elog (or ereport with errmsg_internal) would save
a little workload for translators. But as you were copying an existing
ereport with a translatable message, there's also an argument for sticking
to that style, and maybe mentioning the question to an eventual committer
who might have a stronger opinion.
I did a small double-take seeing the C range_agg_finalfn being shared
by the SQL range_agg_finalfn and multirange_agg_finalfn. I infer that
the reason it works is get_fn_expr_rettype works equally well with
either parameter type.
Do you think it would be worth adding a comment at the C function
explaining that? In a quick query I just did, I found no other aggregate
final functions sharing a C function that way, so this could be the first.
Regards,
-Chap
On 3/1/22 13:33, Chapman Flack wrote:
I think the 4 lines should suffice, but it looks like this patch was
generated from a rebase of the old one (with three lines) that ended up
putting the new 'range_agg' entry ahead of 'max' in func.sgml, which
position is now baked into the 4 lines of context. :)
You're right, my last rebase messed up the docs. Here it is fixed. Sorry
about that!
I would not change them to actual Assert, which would blow up the whole
process on failure. If it's a genuine "not expected to happen" case,
maybe changing it to elog (or ereport with errmsg_internal) would save
a little workload for translators.
I like the elog solution. I've changed them in both places.
I did a small double-take seeing the C range_agg_finalfn being shared
by the SQL range_agg_finalfn and multirange_agg_finalfn. I infer that
the reason it works is get_fn_expr_rettype works equally well with
either parameter type.Do you think it would be worth adding a comment at the C function
explaining that? In a quick query I just did, I found no other aggregate
final functions sharing a C function that way, so this could be the first.
I see 13 other shared finalfns (using select
array_agg(aggfnoid::regproc) as procs, array_agg(aggtransfn) as
transfns, aggfinalfn from pg_aggregate where aggfinalfn is distinct from
0 group by aggfinalfn having count(*) > 1;) but a comment can't hurt! Added.
Thanks,
--
Paul ~{:-)
pj@illuminatedcomputing.com
Attachments:
v3-0001-Add-range_agg-with-multirange-inputs.patchtext/x-patch; charset=UTF-8; name=v3-0001-Add-range_agg-with-multirange-inputs.patchDownload
From 1e7a96668b0ad341d29281055927ad693c656843 Mon Sep 17 00:00:00 2001
From: "Paul A. Jungwirth" <pj@illuminatedcomputing.com>
Date: Fri, 10 Dec 2021 16:04:57 -0800
Subject: [PATCH v3] Add range_agg with multirange inputs
---
doc/src/sgml/func.sgml | 45 ++++++++
src/backend/utils/adt/multirangetypes.c | 66 +++++++++++-
src/include/catalog/pg_aggregate.dat | 3 +
src/include/catalog/pg_proc.dat | 11 ++
src/test/regress/expected/multirangetypes.out | 100 ++++++++++++++++++
src/test/regress/expected/opr_sanity.out | 1 +
src/test/regress/sql/multirangetypes.sql | 21 ++++
src/test/regress/sql/opr_sanity.sql | 1 +
8 files changed, 244 insertions(+), 4 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index df3cd5987b..fb1963a687 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -20012,8 +20012,23 @@ SELECT NULLIF(value, '(none)') ...
</para></entry>
<entry>No</entry>
</row>
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>range_agg</primary>
+ </indexterm>
+ <function>range_agg</function> ( <parameter>value</parameter>
+ <type>anymultirange</type> )
+ <returnvalue>anymultirange</returnvalue>
+ </para>
+ <para>
+ Computes the union of the non-null input values.
+ </para></entry>
+ <entry>No</entry>
+ </row>
+
<row>
<entry role="func_table_entry"><para role="func_signature">
<indexterm>
<primary>range_intersect_agg</primary>
@@ -20027,8 +20042,38 @@ SELECT NULLIF(value, '(none)') ...
</para></entry>
<entry>No</entry>
</row>
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>range_intersect_agg</primary>
+ </indexterm>
+ <function>range_intersect_agg</function> ( <parameter>value</parameter>
+ <type>anymultirange</type> )
+ <returnvalue>anymultirange</returnvalue>
+ </para>
+ <para>
+ Computes the intersection of the non-null input values.
+ </para></entry>
+ <entry>No</entry>
+ </row>
+
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>range_intersect_agg</primary>
+ </indexterm>
+ <function>range_intersect_agg</function> ( <parameter>value</parameter>
+ <type>anymultirange</type> )
+ <returnvalue>anymultirange</returnvalue>
+ </para>
+ <para>
+ Computes the intersection of the non-null input values.
+ </para></entry>
+ <entry>No</entry>
+ </row>
+
<row>
<entry role="func_table_entry"><para role="func_signature">
<indexterm>
<primary>string_agg</primary>
diff --git a/src/backend/utils/adt/multirangetypes.c b/src/backend/utils/adt/multirangetypes.c
index c474b24431..5a4c00457a 100644
--- a/src/backend/utils/adt/multirangetypes.c
+++ b/src/backend/utils/adt/multirangetypes.c
@@ -1346,9 +1346,9 @@ range_agg_transfn(PG_FUNCTION_ARGS)
rngtypoid = get_fn_expr_argtype(fcinfo->flinfo, 1);
if (!type_is_range(rngtypoid))
ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("range_agg must be called with a range")));
+ errmsg("range_agg must be called with a range or multirange")));
if (PG_ARGISNULL(0))
state = initArrayResult(rngtypoid, aggContext, false);
else
@@ -1362,8 +1362,10 @@ range_agg_transfn(PG_FUNCTION_ARGS)
}
/*
* range_agg_finalfn: use our internal array to merge touching ranges.
+ *
+ * Shared by range_agg(anyrange) and range_agg(anymultirange).
*/
Datum
range_agg_finalfn(PG_FUNCTION_ARGS)
{
@@ -1397,8 +1399,66 @@ range_agg_finalfn(PG_FUNCTION_ARGS)
PG_RETURN_MULTIRANGE_P(make_multirange(mltrngtypoid, typcache->rngtype, range_count, ranges));
}
+/*
+ * multirange_agg_transfn: combine adjacent/overlapping multiranges.
+ *
+ * All we do here is gather the input multiranges' ranges into an array
+ * so that the finalfn can sort and combine them.
+ */
+Datum
+multirange_agg_transfn(PG_FUNCTION_ARGS)
+{
+ MemoryContext aggContext;
+ Oid mltrngtypoid;
+ TypeCacheEntry *typcache;
+ TypeCacheEntry *rngtypcache;
+ ArrayBuildState *state;
+ MultirangeType *current;
+ int32 range_count;
+ RangeType **ranges;
+ int32 i;
+
+ if (!AggCheckCallContext(fcinfo, &aggContext))
+ elog(ERROR, "multirange_agg_transfn called in non-aggregate context");
+
+ mltrngtypoid = get_fn_expr_argtype(fcinfo->flinfo, 1);
+ if (!type_is_multirange(mltrngtypoid))
+ elog(ERROR, "range_agg must be called with a range or multirange");
+
+ typcache = multirange_get_typcache(fcinfo, mltrngtypoid);
+ rngtypcache = typcache->rngtype;
+
+ if (PG_ARGISNULL(0))
+ state = initArrayResult(rngtypcache->type_id, aggContext, false);
+ else
+ state = (ArrayBuildState *) PG_GETARG_POINTER(0);
+
+ /* skip NULLs */
+ if (!PG_ARGISNULL(1))
+ {
+ current = PG_GETARG_MULTIRANGE_P(1);
+ multirange_deserialize(rngtypcache, current, &range_count, &ranges);
+ if (range_count == 0)
+ {
+ /*
+ * Add an empty range so we get an empty result (not a null result).
+ */
+ accumArrayResult(state,
+ RangeTypePGetDatum(make_empty_range(rngtypcache)),
+ false, rngtypcache->type_id, aggContext);
+ }
+ else
+ {
+ for (i = 0; i < range_count; i++)
+ accumArrayResult(state, RangeTypePGetDatum(ranges[i]), false, rngtypcache->type_id, aggContext);
+ }
+ }
+
+ PG_RETURN_POINTER(state);
+}
+
Datum
multirange_intersect_agg_transfn(PG_FUNCTION_ARGS)
{
MemoryContext aggContext;
@@ -1415,11 +1475,9 @@ multirange_intersect_agg_transfn(PG_FUNCTION_ARGS)
elog(ERROR, "multirange_intersect_agg_transfn called in non-aggregate context");
mltrngtypoid = get_fn_expr_argtype(fcinfo->flinfo, 1);
if (!type_is_multirange(mltrngtypoid))
- ereport(ERROR,
- (errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("range_intersect_agg must be called with a multirange")));
+ elog(ERROR, "range_agg must be called with a range or multirange");
typcache = multirange_get_typcache(fcinfo, mltrngtypoid);
/* strictness ensures these are non-null */
diff --git a/src/include/catalog/pg_aggregate.dat b/src/include/catalog/pg_aggregate.dat
index 2843f4b415..15b4e6461a 100644
--- a/src/include/catalog/pg_aggregate.dat
+++ b/src/include/catalog/pg_aggregate.dat
@@ -562,8 +562,11 @@
aggtranstype => 'anymultirange' },
{ aggfnoid => 'range_agg(anyrange)', aggtransfn => 'range_agg_transfn',
aggfinalfn => 'range_agg_finalfn', aggfinalextra => 't',
aggtranstype => 'internal' },
+{ aggfnoid => 'range_agg(anymultirange)', aggtransfn => 'multirange_agg_transfn',
+ aggfinalfn => 'multirange_agg_finalfn', aggfinalextra => 't',
+ aggtranstype => 'internal' },
# json
{ aggfnoid => 'json_agg', aggtransfn => 'json_agg_transfn',
aggfinalfn => 'json_agg_finalfn', aggtranstype => 'internal' },
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index bf88858171..1f0da00a26 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -10611,8 +10611,19 @@
{ oid => '4301', descr => 'combine aggregate input into a multirange',
proname => 'range_agg', prokind => 'a', proisstrict => 'f',
prorettype => 'anymultirange', proargtypes => 'anyrange',
prosrc => 'aggregate_dummy' },
+{ oid => '8000', descr => 'aggregate transition function',
+ proname => 'multirange_agg_transfn', proisstrict => 'f', prorettype => 'internal',
+ proargtypes => 'internal anymultirange', prosrc => 'multirange_agg_transfn' },
+{ oid => '8001', descr => 'aggregate final function',
+ proname => 'multirange_agg_finalfn', proisstrict => 'f',
+ prorettype => 'anymultirange', proargtypes => 'internal anymultirange',
+ prosrc => 'range_agg_finalfn' },
+{ oid => '8002', descr => 'combine aggregate input into a multirange',
+ proname => 'range_agg', prokind => 'a', proisstrict => 'f',
+ prorettype => 'anymultirange', proargtypes => 'anymultirange',
+ prosrc => 'aggregate_dummy' },
{ oid => '4388', descr => 'range aggregate by intersecting',
proname => 'multirange_intersect_agg_transfn', prorettype => 'anymultirange',
proargtypes => 'anymultirange anymultirange',
prosrc => 'multirange_intersect_agg_transfn' },
diff --git a/src/test/regress/expected/multirangetypes.out b/src/test/regress/expected/multirangetypes.out
index bde3f248a7..ac2eb84c3a 100644
--- a/src/test/regress/expected/multirangetypes.out
+++ b/src/test/regress/expected/multirangetypes.out
@@ -2783,8 +2783,72 @@ FROM (VALUES
---------------
{[a,f],[g,j)}
(1 row)
+-- range_agg with multirange inputs
+select range_agg(nmr) from nummultirange_test;
+ range_agg
+-----------
+ {(,)}
+(1 row)
+
+select range_agg(nmr) from nummultirange_test where false;
+ range_agg
+-----------
+
+(1 row)
+
+select range_agg(null::nummultirange) from nummultirange_test;
+ range_agg
+-----------
+
+(1 row)
+
+select range_agg(nmr) from (values ('{}'::nummultirange)) t(nmr);
+ range_agg
+-----------
+ {}
+(1 row)
+
+select range_agg(nmr) from (values ('{}'::nummultirange), ('{}'::nummultirange)) t(nmr);
+ range_agg
+-----------
+ {}
+(1 row)
+
+select range_agg(nmr) from (values ('{[1,2]}'::nummultirange)) t(nmr);
+ range_agg
+-----------
+ {[1,2]}
+(1 row)
+
+select range_agg(nmr) from (values ('{[1,2], [5,6]}'::nummultirange)) t(nmr);
+ range_agg
+---------------
+ {[1,2],[5,6]}
+(1 row)
+
+select range_agg(nmr) from (values ('{[1,2], [2,3]}'::nummultirange)) t(nmr);
+ range_agg
+-----------
+ {[1,3]}
+(1 row)
+
+select range_agg(nmr) from (values ('{[1,2]}'::nummultirange), ('{[5,6]}'::nummultirange)) t(nmr);
+ range_agg
+---------------
+ {[1,2],[5,6]}
+(1 row)
+
+select range_agg(nmr) from (values ('{[1,2]}'::nummultirange), ('{[2,3]}'::nummultirange)) t(nmr);
+ range_agg
+-----------
+ {[1,3]}
+(1 row)
+
+--
+-- range_intersect_agg function
+--
select range_intersect_agg(nmr) from nummultirange_test;
range_intersect_agg
---------------------
{}
@@ -2795,15 +2859,51 @@ select range_intersect_agg(nmr) from nummultirange_test where false;
---------------------
(1 row)
+select range_intersect_agg(null::nummultirange) from nummultirange_test;
+ range_intersect_agg
+---------------------
+
+(1 row)
+
+select range_intersect_agg(nmr) from (values ('{[1,3]}'::nummultirange), ('{[6,12]}'::nummultirange)) t(nmr);
+ range_intersect_agg
+---------------------
+ {}
+(1 row)
+
+select range_intersect_agg(nmr) from (values ('{[1,6]}'::nummultirange), ('{[3,12]}'::nummultirange)) t(nmr);
+ range_intersect_agg
+---------------------
+ {[3,6]}
+(1 row)
+
+select range_intersect_agg(nmr) from (values ('{[1,6], [10,12]}'::nummultirange), ('{[4,14]}'::nummultirange)) t(nmr);
+ range_intersect_agg
+---------------------
+ {[4,6],[10,12]}
+(1 row)
+
-- test with just one input:
+select range_intersect_agg(nmr) from (values ('{}'::nummultirange)) t(nmr);
+ range_intersect_agg
+---------------------
+ {}
+(1 row)
+
select range_intersect_agg(nmr) from (values ('{[1,2]}'::nummultirange)) t(nmr);
range_intersect_agg
---------------------
{[1,2]}
(1 row)
+select range_intersect_agg(nmr) from (values ('{[1,6], [10,12]}'::nummultirange)) t(nmr);
+ range_intersect_agg
+---------------------
+ {[1,6],[10,12]}
+(1 row)
+
select range_intersect_agg(nmr) from nummultirange_test where nmr @> 4.0;
range_intersect_agg
---------------------
{[3,5)}
diff --git a/src/test/regress/expected/opr_sanity.out b/src/test/regress/expected/opr_sanity.out
index 4ce6c039b4..046d2006d5 100644
--- a/src/test/regress/expected/opr_sanity.out
+++ b/src/test/regress/expected/opr_sanity.out
@@ -192,8 +192,9 @@ WHERE p1.oid != p2.oid AND
p1.prosrc NOT LIKE E'range\\_constructor_' AND
p2.prosrc NOT LIKE E'range\\_constructor_' AND
p1.prosrc NOT LIKE E'multirange\\_constructor_' AND
p2.prosrc NOT LIKE E'multirange\\_constructor_' AND
+ p2.prosrc NOT LIKE E'range\\_agg\\_finalfn' AND
(p1.proargtypes[1] < p2.proargtypes[1])
ORDER BY 1, 2;
proargtypes | proargtypes
-----------------------------+--------------------------
diff --git a/src/test/regress/sql/multirangetypes.sql b/src/test/regress/sql/multirangetypes.sql
index df1edb4d31..1abcaeddb5 100644
--- a/src/test/regress/sql/multirangetypes.sql
+++ b/src/test/regress/sql/multirangetypes.sql
@@ -571,12 +571,33 @@ FROM (VALUES
('[g,h)'::textrange),
('[h,j)'::textrange)
) t(r);
+-- range_agg with multirange inputs
+select range_agg(nmr) from nummultirange_test;
+select range_agg(nmr) from nummultirange_test where false;
+select range_agg(null::nummultirange) from nummultirange_test;
+select range_agg(nmr) from (values ('{}'::nummultirange)) t(nmr);
+select range_agg(nmr) from (values ('{}'::nummultirange), ('{}'::nummultirange)) t(nmr);
+select range_agg(nmr) from (values ('{[1,2]}'::nummultirange)) t(nmr);
+select range_agg(nmr) from (values ('{[1,2], [5,6]}'::nummultirange)) t(nmr);
+select range_agg(nmr) from (values ('{[1,2], [2,3]}'::nummultirange)) t(nmr);
+select range_agg(nmr) from (values ('{[1,2]}'::nummultirange), ('{[5,6]}'::nummultirange)) t(nmr);
+select range_agg(nmr) from (values ('{[1,2]}'::nummultirange), ('{[2,3]}'::nummultirange)) t(nmr);
+
+--
+-- range_intersect_agg function
+--
select range_intersect_agg(nmr) from nummultirange_test;
select range_intersect_agg(nmr) from nummultirange_test where false;
+select range_intersect_agg(null::nummultirange) from nummultirange_test;
+select range_intersect_agg(nmr) from (values ('{[1,3]}'::nummultirange), ('{[6,12]}'::nummultirange)) t(nmr);
+select range_intersect_agg(nmr) from (values ('{[1,6]}'::nummultirange), ('{[3,12]}'::nummultirange)) t(nmr);
+select range_intersect_agg(nmr) from (values ('{[1,6], [10,12]}'::nummultirange), ('{[4,14]}'::nummultirange)) t(nmr);
-- test with just one input:
+select range_intersect_agg(nmr) from (values ('{}'::nummultirange)) t(nmr);
select range_intersect_agg(nmr) from (values ('{[1,2]}'::nummultirange)) t(nmr);
+select range_intersect_agg(nmr) from (values ('{[1,6], [10,12]}'::nummultirange)) t(nmr);
select range_intersect_agg(nmr) from nummultirange_test where nmr @> 4.0;
create table nummultirange_test2(nmr nummultirange);
create index nummultirange_test2_hash_idx on nummultirange_test2 using hash (nmr);
diff --git a/src/test/regress/sql/opr_sanity.sql b/src/test/regress/sql/opr_sanity.sql
index 2b292851e3..bbcffda228 100644
--- a/src/test/regress/sql/opr_sanity.sql
+++ b/src/test/regress/sql/opr_sanity.sql
@@ -152,8 +152,9 @@ WHERE p1.oid != p2.oid AND
p1.prosrc NOT LIKE E'range\\_constructor_' AND
p2.prosrc NOT LIKE E'range\\_constructor_' AND
p1.prosrc NOT LIKE E'multirange\\_constructor_' AND
p2.prosrc NOT LIKE E'multirange\\_constructor_' AND
+ p2.prosrc NOT LIKE E'range\\_agg\\_finalfn' AND
(p1.proargtypes[1] < p2.proargtypes[1])
ORDER BY 1, 2;
SELECT DISTINCT p1.proargtypes[2]::regtype, p2.proargtypes[2]::regtype
--
2.25.1
On 03/05/22 15:53, Paul Jungwirth wrote:
On 3/1/22 13:33, Chapman Flack wrote:
I think the 4 lines should suffice, but it looks like this patch was
generated from a rebase of the old one (with three lines) that ended up
putting the new 'range_agg' entry ahead of 'max' in func.sgml, which
position is now baked into the 4 lines of context. :)You're right, my last rebase messed up the docs. Here it is fixed. Sorry
about that!
When I apply this patch, I get a func.sgml with two entries for
range_intersect_agg(anymultirange).
I like the elog solution. I've changed them in both places.
It looks like you've now got elog in three places: the "must be called
with a range or multirange" in multirange_agg_transfn and
multirange_intersect_agg_transfn, and the "called in non-aggregate
context" in multirange_agg_transfn.
I think that last is also ok, given that its state type is internal,
so it shouldn't be reachable in a user call.
In range_agg_transfn, you've changed the message in the "must be called
with a range or multirange"; that seems like another good candidate to
be an elog.
I see 13 other shared finalfns (using select array_agg(aggfnoid::regproc) as
procs, array_agg(aggtransfn) as transfns, aggfinalfn from pg_aggregate where
aggfinalfn is distinct from 0 group by aggfinalfn having count(*) > 1;) but
a comment can't hurt! Added.
I think your query finds aggregate declarations that share the same SQL
function declaration as their finalizer functions. That seems to be more
common.
The query I used looks for cases where different SQL-declared functions
appear as finalizers of aggregates, but the different SQL declared functions
share the same internal C implementation. That's the query where this seems
to be the unique result.
WITH
finals(regp) AS (
SELECT DISTINCT
CAST(aggfinalfn AS regprocedure)
FROM
pg_aggregate
WHERE
aggfinalfn <> 0 -- InvalidOid
)
SELECT
prosrc, array_agg(regp)
FROM
pg_proc, finals
WHERE
oid = regp AND prolang = 12 -- INTERNALlanguageId
GROUP BY
prosrc
HAVING
count(*) > 1;
In other words, I think the interesting thing to say in the C comment
is not "shared by range_agg(anyrange) and range_agg(anymultirange)", but
"shared by range_agg_finalfn(internal,anyrange) and
multirange_agg_finalfn(internal,anymultirange)".
It seems a little extra surprising to have one C function declared in SQL
with two different names and parameter signatures. It ends up working
out because it relies on get_fn_expr_rettype, which can do its job for
either polymorphic type it might find in the parameter declaration.
But that's a bit subtle. :)
Regards,
-Chap
On 3/10/22 14:07, Chapman Flack wrote:
When I apply this patch, I get a func.sgml with two entries for
range_intersect_agg(anymultirange).
Arg, fixed.
In range_agg_transfn, you've changed the message in the "must be called
with a range or multirange"; that seems like another good candidate to
be an elog.
Agreed. Updated here.
I think your query finds aggregate declarations that share the same SQL
function declaration as their finalizer functions. That seems to be more
common.The query I used looks for cases where different SQL-declared functions
appear as finalizers of aggregates, but the different SQL declared functions
share the same internal C implementation.
Okay, I see. I believe that is quite common for ordinary SQL functions.
Sharing a prosrc seems even less remarkable than sharing an aggfinalfn.
You're right there are no cases for other finalfns yet, but I don't
think there is anything special about finalfns that would make this a
weirder thing to do there than with ordinary functions. Still, noting it
with a comment does seem helpful. I've updated the remark to match what
you suggested.
Thank you again for the review, and sorry for so many iterations! :-)
Yours,
--
Paul ~{:-)
pj@illuminatedcomputing.com
Attachments:
v4-0001-Add-range_agg-with-multirange-inputs.patchtext/x-patch; charset=UTF-8; name=v4-0001-Add-range_agg-with-multirange-inputs.patchDownload
From b409d7333132e34a928ec8cc0ecca0a3421b7268 Mon Sep 17 00:00:00 2001
From: "Paul A. Jungwirth" <pj@illuminatedcomputing.com>
Date: Fri, 10 Dec 2021 16:04:57 -0800
Subject: [PATCH v4] Add range_agg with multirange inputs
---
doc/src/sgml/func.sgml | 30 ++++++
src/backend/utils/adt/multirangetypes.c | 69 ++++++++++--
src/include/catalog/pg_aggregate.dat | 3 +
src/include/catalog/pg_proc.dat | 11 ++
src/test/regress/expected/multirangetypes.out | 100 ++++++++++++++++++
src/test/regress/expected/opr_sanity.out | 1 +
src/test/regress/sql/multirangetypes.sql | 21 ++++
src/test/regress/sql/opr_sanity.sql | 1 +
8 files changed, 230 insertions(+), 6 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index df3cd5987b..ab6c4f0093 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -20012,8 +20012,23 @@ SELECT NULLIF(value, '(none)') ...
</para></entry>
<entry>No</entry>
</row>
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>range_agg</primary>
+ </indexterm>
+ <function>range_agg</function> ( <parameter>value</parameter>
+ <type>anymultirange</type> )
+ <returnvalue>anymultirange</returnvalue>
+ </para>
+ <para>
+ Computes the union of the non-null input values.
+ </para></entry>
+ <entry>No</entry>
+ </row>
+
<row>
<entry role="func_table_entry"><para role="func_signature">
<indexterm>
<primary>range_intersect_agg</primary>
@@ -20027,8 +20042,23 @@ SELECT NULLIF(value, '(none)') ...
</para></entry>
<entry>No</entry>
</row>
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>range_intersect_agg</primary>
+ </indexterm>
+ <function>range_intersect_agg</function> ( <parameter>value</parameter>
+ <type>anymultirange</type> )
+ <returnvalue>anymultirange</returnvalue>
+ </para>
+ <para>
+ Computes the intersection of the non-null input values.
+ </para></entry>
+ <entry>No</entry>
+ </row>
+
<row>
<entry role="func_table_entry"><para role="func_signature">
<indexterm>
<primary>string_agg</primary>
diff --git a/src/backend/utils/adt/multirangetypes.c b/src/backend/utils/adt/multirangetypes.c
index c474b24431..a6d376b083 100644
--- a/src/backend/utils/adt/multirangetypes.c
+++ b/src/backend/utils/adt/multirangetypes.c
@@ -1344,11 +1344,9 @@ range_agg_transfn(PG_FUNCTION_ARGS)
elog(ERROR, "range_agg_transfn called in non-aggregate context");
rngtypoid = get_fn_expr_argtype(fcinfo->flinfo, 1);
if (!type_is_range(rngtypoid))
- ereport(ERROR,
- (errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("range_agg must be called with a range")));
+ elog(ERROR, "range_agg must be called with a range or multirange");
if (PG_ARGISNULL(0))
state = initArrayResult(rngtypoid, aggContext, false);
else
@@ -1362,8 +1360,11 @@ range_agg_transfn(PG_FUNCTION_ARGS)
}
/*
* range_agg_finalfn: use our internal array to merge touching ranges.
+ *
+ * Shared by range_agg_finalfn(anyrange) and
+ * multirange_agg_finalfn(anymultirange).
*/
Datum
range_agg_finalfn(PG_FUNCTION_ARGS)
{
@@ -1397,8 +1398,66 @@ range_agg_finalfn(PG_FUNCTION_ARGS)
PG_RETURN_MULTIRANGE_P(make_multirange(mltrngtypoid, typcache->rngtype, range_count, ranges));
}
+/*
+ * multirange_agg_transfn: combine adjacent/overlapping multiranges.
+ *
+ * All we do here is gather the input multiranges' ranges into an array
+ * so that the finalfn can sort and combine them.
+ */
+Datum
+multirange_agg_transfn(PG_FUNCTION_ARGS)
+{
+ MemoryContext aggContext;
+ Oid mltrngtypoid;
+ TypeCacheEntry *typcache;
+ TypeCacheEntry *rngtypcache;
+ ArrayBuildState *state;
+ MultirangeType *current;
+ int32 range_count;
+ RangeType **ranges;
+ int32 i;
+
+ if (!AggCheckCallContext(fcinfo, &aggContext))
+ elog(ERROR, "multirange_agg_transfn called in non-aggregate context");
+
+ mltrngtypoid = get_fn_expr_argtype(fcinfo->flinfo, 1);
+ if (!type_is_multirange(mltrngtypoid))
+ elog(ERROR, "range_agg must be called with a range or multirange");
+
+ typcache = multirange_get_typcache(fcinfo, mltrngtypoid);
+ rngtypcache = typcache->rngtype;
+
+ if (PG_ARGISNULL(0))
+ state = initArrayResult(rngtypcache->type_id, aggContext, false);
+ else
+ state = (ArrayBuildState *) PG_GETARG_POINTER(0);
+
+ /* skip NULLs */
+ if (!PG_ARGISNULL(1))
+ {
+ current = PG_GETARG_MULTIRANGE_P(1);
+ multirange_deserialize(rngtypcache, current, &range_count, &ranges);
+ if (range_count == 0)
+ {
+ /*
+ * Add an empty range so we get an empty result (not a null result).
+ */
+ accumArrayResult(state,
+ RangeTypePGetDatum(make_empty_range(rngtypcache)),
+ false, rngtypcache->type_id, aggContext);
+ }
+ else
+ {
+ for (i = 0; i < range_count; i++)
+ accumArrayResult(state, RangeTypePGetDatum(ranges[i]), false, rngtypcache->type_id, aggContext);
+ }
+ }
+
+ PG_RETURN_POINTER(state);
+}
+
Datum
multirange_intersect_agg_transfn(PG_FUNCTION_ARGS)
{
MemoryContext aggContext;
@@ -1415,11 +1474,9 @@ multirange_intersect_agg_transfn(PG_FUNCTION_ARGS)
elog(ERROR, "multirange_intersect_agg_transfn called in non-aggregate context");
mltrngtypoid = get_fn_expr_argtype(fcinfo->flinfo, 1);
if (!type_is_multirange(mltrngtypoid))
- ereport(ERROR,
- (errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("range_intersect_agg must be called with a multirange")));
+ elog(ERROR, "range_agg must be called with a range or multirange");
typcache = multirange_get_typcache(fcinfo, mltrngtypoid);
/* strictness ensures these are non-null */
diff --git a/src/include/catalog/pg_aggregate.dat b/src/include/catalog/pg_aggregate.dat
index 2843f4b415..15b4e6461a 100644
--- a/src/include/catalog/pg_aggregate.dat
+++ b/src/include/catalog/pg_aggregate.dat
@@ -562,8 +562,11 @@
aggtranstype => 'anymultirange' },
{ aggfnoid => 'range_agg(anyrange)', aggtransfn => 'range_agg_transfn',
aggfinalfn => 'range_agg_finalfn', aggfinalextra => 't',
aggtranstype => 'internal' },
+{ aggfnoid => 'range_agg(anymultirange)', aggtransfn => 'multirange_agg_transfn',
+ aggfinalfn => 'multirange_agg_finalfn', aggfinalextra => 't',
+ aggtranstype => 'internal' },
# json
{ aggfnoid => 'json_agg', aggtransfn => 'json_agg_transfn',
aggfinalfn => 'json_agg_finalfn', aggtranstype => 'internal' },
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index bf88858171..1f0da00a26 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -10611,8 +10611,19 @@
{ oid => '4301', descr => 'combine aggregate input into a multirange',
proname => 'range_agg', prokind => 'a', proisstrict => 'f',
prorettype => 'anymultirange', proargtypes => 'anyrange',
prosrc => 'aggregate_dummy' },
+{ oid => '8000', descr => 'aggregate transition function',
+ proname => 'multirange_agg_transfn', proisstrict => 'f', prorettype => 'internal',
+ proargtypes => 'internal anymultirange', prosrc => 'multirange_agg_transfn' },
+{ oid => '8001', descr => 'aggregate final function',
+ proname => 'multirange_agg_finalfn', proisstrict => 'f',
+ prorettype => 'anymultirange', proargtypes => 'internal anymultirange',
+ prosrc => 'range_agg_finalfn' },
+{ oid => '8002', descr => 'combine aggregate input into a multirange',
+ proname => 'range_agg', prokind => 'a', proisstrict => 'f',
+ prorettype => 'anymultirange', proargtypes => 'anymultirange',
+ prosrc => 'aggregate_dummy' },
{ oid => '4388', descr => 'range aggregate by intersecting',
proname => 'multirange_intersect_agg_transfn', prorettype => 'anymultirange',
proargtypes => 'anymultirange anymultirange',
prosrc => 'multirange_intersect_agg_transfn' },
diff --git a/src/test/regress/expected/multirangetypes.out b/src/test/regress/expected/multirangetypes.out
index bde3f248a7..ac2eb84c3a 100644
--- a/src/test/regress/expected/multirangetypes.out
+++ b/src/test/regress/expected/multirangetypes.out
@@ -2783,8 +2783,72 @@ FROM (VALUES
---------------
{[a,f],[g,j)}
(1 row)
+-- range_agg with multirange inputs
+select range_agg(nmr) from nummultirange_test;
+ range_agg
+-----------
+ {(,)}
+(1 row)
+
+select range_agg(nmr) from nummultirange_test where false;
+ range_agg
+-----------
+
+(1 row)
+
+select range_agg(null::nummultirange) from nummultirange_test;
+ range_agg
+-----------
+
+(1 row)
+
+select range_agg(nmr) from (values ('{}'::nummultirange)) t(nmr);
+ range_agg
+-----------
+ {}
+(1 row)
+
+select range_agg(nmr) from (values ('{}'::nummultirange), ('{}'::nummultirange)) t(nmr);
+ range_agg
+-----------
+ {}
+(1 row)
+
+select range_agg(nmr) from (values ('{[1,2]}'::nummultirange)) t(nmr);
+ range_agg
+-----------
+ {[1,2]}
+(1 row)
+
+select range_agg(nmr) from (values ('{[1,2], [5,6]}'::nummultirange)) t(nmr);
+ range_agg
+---------------
+ {[1,2],[5,6]}
+(1 row)
+
+select range_agg(nmr) from (values ('{[1,2], [2,3]}'::nummultirange)) t(nmr);
+ range_agg
+-----------
+ {[1,3]}
+(1 row)
+
+select range_agg(nmr) from (values ('{[1,2]}'::nummultirange), ('{[5,6]}'::nummultirange)) t(nmr);
+ range_agg
+---------------
+ {[1,2],[5,6]}
+(1 row)
+
+select range_agg(nmr) from (values ('{[1,2]}'::nummultirange), ('{[2,3]}'::nummultirange)) t(nmr);
+ range_agg
+-----------
+ {[1,3]}
+(1 row)
+
+--
+-- range_intersect_agg function
+--
select range_intersect_agg(nmr) from nummultirange_test;
range_intersect_agg
---------------------
{}
@@ -2795,15 +2859,51 @@ select range_intersect_agg(nmr) from nummultirange_test where false;
---------------------
(1 row)
+select range_intersect_agg(null::nummultirange) from nummultirange_test;
+ range_intersect_agg
+---------------------
+
+(1 row)
+
+select range_intersect_agg(nmr) from (values ('{[1,3]}'::nummultirange), ('{[6,12]}'::nummultirange)) t(nmr);
+ range_intersect_agg
+---------------------
+ {}
+(1 row)
+
+select range_intersect_agg(nmr) from (values ('{[1,6]}'::nummultirange), ('{[3,12]}'::nummultirange)) t(nmr);
+ range_intersect_agg
+---------------------
+ {[3,6]}
+(1 row)
+
+select range_intersect_agg(nmr) from (values ('{[1,6], [10,12]}'::nummultirange), ('{[4,14]}'::nummultirange)) t(nmr);
+ range_intersect_agg
+---------------------
+ {[4,6],[10,12]}
+(1 row)
+
-- test with just one input:
+select range_intersect_agg(nmr) from (values ('{}'::nummultirange)) t(nmr);
+ range_intersect_agg
+---------------------
+ {}
+(1 row)
+
select range_intersect_agg(nmr) from (values ('{[1,2]}'::nummultirange)) t(nmr);
range_intersect_agg
---------------------
{[1,2]}
(1 row)
+select range_intersect_agg(nmr) from (values ('{[1,6], [10,12]}'::nummultirange)) t(nmr);
+ range_intersect_agg
+---------------------
+ {[1,6],[10,12]}
+(1 row)
+
select range_intersect_agg(nmr) from nummultirange_test where nmr @> 4.0;
range_intersect_agg
---------------------
{[3,5)}
diff --git a/src/test/regress/expected/opr_sanity.out b/src/test/regress/expected/opr_sanity.out
index 4ce6c039b4..046d2006d5 100644
--- a/src/test/regress/expected/opr_sanity.out
+++ b/src/test/regress/expected/opr_sanity.out
@@ -192,8 +192,9 @@ WHERE p1.oid != p2.oid AND
p1.prosrc NOT LIKE E'range\\_constructor_' AND
p2.prosrc NOT LIKE E'range\\_constructor_' AND
p1.prosrc NOT LIKE E'multirange\\_constructor_' AND
p2.prosrc NOT LIKE E'multirange\\_constructor_' AND
+ p2.prosrc NOT LIKE E'range\\_agg\\_finalfn' AND
(p1.proargtypes[1] < p2.proargtypes[1])
ORDER BY 1, 2;
proargtypes | proargtypes
-----------------------------+--------------------------
diff --git a/src/test/regress/sql/multirangetypes.sql b/src/test/regress/sql/multirangetypes.sql
index df1edb4d31..1abcaeddb5 100644
--- a/src/test/regress/sql/multirangetypes.sql
+++ b/src/test/regress/sql/multirangetypes.sql
@@ -571,12 +571,33 @@ FROM (VALUES
('[g,h)'::textrange),
('[h,j)'::textrange)
) t(r);
+-- range_agg with multirange inputs
+select range_agg(nmr) from nummultirange_test;
+select range_agg(nmr) from nummultirange_test where false;
+select range_agg(null::nummultirange) from nummultirange_test;
+select range_agg(nmr) from (values ('{}'::nummultirange)) t(nmr);
+select range_agg(nmr) from (values ('{}'::nummultirange), ('{}'::nummultirange)) t(nmr);
+select range_agg(nmr) from (values ('{[1,2]}'::nummultirange)) t(nmr);
+select range_agg(nmr) from (values ('{[1,2], [5,6]}'::nummultirange)) t(nmr);
+select range_agg(nmr) from (values ('{[1,2], [2,3]}'::nummultirange)) t(nmr);
+select range_agg(nmr) from (values ('{[1,2]}'::nummultirange), ('{[5,6]}'::nummultirange)) t(nmr);
+select range_agg(nmr) from (values ('{[1,2]}'::nummultirange), ('{[2,3]}'::nummultirange)) t(nmr);
+
+--
+-- range_intersect_agg function
+--
select range_intersect_agg(nmr) from nummultirange_test;
select range_intersect_agg(nmr) from nummultirange_test where false;
+select range_intersect_agg(null::nummultirange) from nummultirange_test;
+select range_intersect_agg(nmr) from (values ('{[1,3]}'::nummultirange), ('{[6,12]}'::nummultirange)) t(nmr);
+select range_intersect_agg(nmr) from (values ('{[1,6]}'::nummultirange), ('{[3,12]}'::nummultirange)) t(nmr);
+select range_intersect_agg(nmr) from (values ('{[1,6], [10,12]}'::nummultirange), ('{[4,14]}'::nummultirange)) t(nmr);
-- test with just one input:
+select range_intersect_agg(nmr) from (values ('{}'::nummultirange)) t(nmr);
select range_intersect_agg(nmr) from (values ('{[1,2]}'::nummultirange)) t(nmr);
+select range_intersect_agg(nmr) from (values ('{[1,6], [10,12]}'::nummultirange)) t(nmr);
select range_intersect_agg(nmr) from nummultirange_test where nmr @> 4.0;
create table nummultirange_test2(nmr nummultirange);
create index nummultirange_test2_hash_idx on nummultirange_test2 using hash (nmr);
diff --git a/src/test/regress/sql/opr_sanity.sql b/src/test/regress/sql/opr_sanity.sql
index 2b292851e3..bbcffda228 100644
--- a/src/test/regress/sql/opr_sanity.sql
+++ b/src/test/regress/sql/opr_sanity.sql
@@ -152,8 +152,9 @@ WHERE p1.oid != p2.oid AND
p1.prosrc NOT LIKE E'range\\_constructor_' AND
p2.prosrc NOT LIKE E'range\\_constructor_' AND
p1.prosrc NOT LIKE E'multirange\\_constructor_' AND
p2.prosrc NOT LIKE E'multirange\\_constructor_' AND
+ p2.prosrc NOT LIKE E'range\\_agg\\_finalfn' AND
(p1.proargtypes[1] < p2.proargtypes[1])
ORDER BY 1, 2;
SELECT DISTINCT p1.proargtypes[2]::regtype, p2.proargtypes[2]::regtype
--
2.25.1
On 03/11/22 22:18, Paul Jungwirth wrote:
Arg, fixed.
In range_agg_transfn, you've changed the message in the "must be called
with a range or multirange"; that seems like another good candidate to
be an elog.Agreed. Updated here.
This looks good to me and passes installcheck-world, so I'll push
the RfC button.
Sharing a prosrc seems even less remarkable than sharing an aggfinalfn.
You're right there are no cases for other finalfns yet, but I don't think
there is anything special about finalfns that would make this a weirder
thing to do there than with ordinary functions.
You sent me back to look at how many of those there are. I get 42 cases
of shared prosrc (43 now).
The chief subgroup of those looks to involve sharing between parameter
signatures where the types have identical layouts and the semantic
differences are unimportant to the function in question (comparisons
between bit or between varbit, overlaps taking timestamp or timestamptz,
etc.).
The other prominent group is range and multirange constructors, where
the C function has an obviously generic name like range_constructor2
and gets shared by a bunch of SQL declarations.
I think here we've added the first instance where the C function is
shared by SQL-declared functions accepting two different polymorphic
pseudotypes. But it's clearly simple and works, nothing objectionable
about it.
I had experimented with renaming multirange_agg_finalfn to just
range_agg_finalfn so it would just look like two overloads of one
function sharing a prosrc, and ultimately gave up because genbki.pl
couldn't resolve the OID where the name is used in pg_aggregate.dat.
That's why it surprised me to see three instances where other functions
(overlaps, isfinite, name) do use the same SQL name for different
overloads, but the explanation seems to be that nothing else at genbki
time refers to those, so genbki's unique-name limitation doesn't affect
them.
Neither here nor there for this patch, but an interesting new thing
I learned while reviewing it.
Regards,
-Chap
Fwiw the cfbot is failing due to a duplicate OID. Traditionally we
didn't treat duplicate OIDs as reason to reject a patch because
they're inevitable as other patches get committed and the committer
can just renumber them.
I think the cfbot kind of changes this calculus since it's a pain lose
the visibility into whether the rest of the tests are passing that the
cfbot normally gives us.
If it's not to much of a hassle could you renumber resubmit the patch
with an updated OID?
[10:54:57.606] su postgres -c "make -s -j${BUILD_JOBS} world-bin"
[10:54:57.927] Duplicate OIDs detected:
[10:54:57.927] 8000
This patch has been committed. I split it into a few pieces.
On 12.03.22 04:18, Paul Jungwirth wrote:
On 3/10/22 14:07, Chapman Flack wrote:
When I apply this patch, I get a func.sgml with two entries for
range_intersect_agg(anymultirange).Arg, fixed.
In range_agg_transfn, you've changed the message in the "must be called
with a range or multirange"; that seems like another good candidate to
be an elog.Agreed. Updated here.
I kept those messages as "range" or "multirange" separately, instead of
"range or multirange". This way, we don't have to update all the
messages of this kind when a new function is added. Since these are
only internal messages anyway, I opted for higher maintainability.