PG18: Fix distributed MIN/MAX for arrays types #8421
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8421 +/- ##
==========================================
+ Coverage 88.78% 88.80% +0.02%
==========================================
Files 287 287
Lines 63257 63275 +18
Branches 7931 7933 +2
==========================================
+ Hits 56163 56194 +31
+ Misses 4758 4747 -11
+ Partials 2336 2334 -2 🚀 New features to boost your workflow:
|
m3hm3t
force-pushed
the
m3hm3t/pg18_minmax_array
branch
from
3806b4b to
e960c5c
Compare
m3hm3t
changed the title
m3hm3t
force-pushed
the
m3hm3t/pg18_minmax_array
branch
2 times, most recently
from
88dd8dd to
49ed52e
Compare
m3hm3t
changed the title
m3hm3t
force-pushed
the
m3hm3t/pg18_minmax_array
branch
2 times, most recently
from
933d3bf to
8cec88d
Compare
naisila
left a comment
naisila
left a comment
There was a problem hiding this comment.
some questions and recommendations for clarifications on this PR as it's a bit unclear to me what we are trying to do
| typedef enum AggregateArgMatchLevel | ||
| { | ||
| AGG_MATCH_NONE = 0, | ||
| AGG_MATCH_RECORD = 1, | ||
| AGG_MATCH_GENERAL_POLY = 2, | ||
| AGG_MATCH_ARRAY_POLY = 3, | ||
| AGG_MATCH_EXACT = 4 | ||
| } AggregateArgMatchLevel; |
There was a problem hiding this comment.
Can we have a test case for each of these AggregateArgMatchLevel? In that way we can validate all cases and better understand the differences.
There was a problem hiding this comment.
Added tests for
AGG_MATCH_RECORD = 1,
AGG_MATCH_GENERAL_POLY = 2,
AGG_MATCH_ARRAY_POLY = 3,
I believe can not add for matach none.
records support will come with another PR after this one is merged.
There was a problem hiding this comment.
record support is AGG_MATCH_RECORD, right ? So if that is tbd the tests in this PR cover AGG_MATCH_EXACT, AGG_MATCH_ARRAY_POLY and AGG_MATCH_GENERAL_POLY ?
There was a problem hiding this comment.
Already added tests for those:
https://github.com/citusdata/citus/pull/8421/changes#diff-c060a0d5de0e3d4aa7a0d2ba5ea08d429cf31512b2a1ba42886c8b2026b88f34R1902
https://github.com/citusdata/citus/pull/8421/changes#diff-c060a0d5de0e3d4aa7a0d2ba5ea08d429cf31512b2a1ba42886c8b2026b88f34R1916
https://github.com/citusdata/citus/pull/8421/changes#diff-c060a0d5de0e3d4aa7a0d2ba5ea08d429cf31512b2a1ba42886c8b2026b88f34R1935
Record support will come with this PR: #8429
or am i missing something?
There was a problem hiding this comment.
So, may be a nit, but the list in the second message of this thread should read:
Added tests for
AGG_MATCH_GENERAL_POLY = 2
AGG_MATCH_ARRAY_POLY = 3
AGG_MATCH_EXACT = 4
958b157 to
2719bad
Compare
m3hm3t
force-pushed
the
m3hm3t/pg18_minmax_array
branch
3 times, most recently
from
caae21c to
42c04c6
Compare
colm-mchugh
left a comment
colm-mchugh
left a comment
There was a problem hiding this comment.
Lgtm overall, couple of minor comments on macro usage and code flow.
…ORD types in distributed tables PG18: Add tests for MIN/MAX aggregate OID resolution on ANYARRAY and RECORD types
m3hm3t
force-pushed
the
m3hm3t/pg18_minmax_array
branch
from
abf7f31 to
a7318e6
Compare
DESCRIPTION: PG18: Fix distributed MIN/MAX for arrays types
fixes #8400
error: #8400 (comment)
PostgreSQL 18 added/expanded
min()/max()support to cover additional type categories (notably composite/record values, and other polymorphic signatures).postgres/postgres@a0f1fce
In Citus, distributed aggregate planning needs to resolve the correct aggregate function OID for the worker-side aggregate based on
(function name, input type). On PG18, that lookup can fail for polymorphic declarations (e.g.,ANYARRAY,RECORD), resulting in errors likeno matching oid for function: minon distributed tables.What this PR changes
Make master aggregate return typing robust for polymorphic aggregates by treating all polymorphic return types via
IsPolymorphicType(...)(instead of onlyANYELEMENT). This ensures the master aggregate’saggtypeis set to the concrete worker return type.Improve aggregate OID resolution by ranking candidate matches and picking the “best” one:
ANYARRAY)ANYELEMENT/ANYENUM)RECORDfor rowtypes)This prevents the PG18 regressions where the old logic only handled exact +
ANYELEMENT.