Change Datum to only perform checked addition

[sgrif]

Datum does not represent a type that can unconditionally add to itself. Currently, any time we encounter two types which do not match, or any types which can't be added, we silently return NULL. This changes that behavior to return a specific error. The Add impl is removed, as these cases will most likely represent a bug in pgdog and shouldn't be silently swallowed.

Two potential future changes stand out to me. The first is that we currently unconditionally error on types that differ, even though we can absolutely add Integer to Bigint for example. I'm not sure that we want to be responsible for maintaining the whole matrix of cross type addition that PG supports, so for the time being I maintained the current behavior.

The second is that how we handle NULL is currently the exact opposite of what PG does. I suspect this will be because all calls to addition come from the implementation of aggregate functions, where we might get NULL from a shard that returned no rows, and we don't want that NULL to cancel out the results from other shards.

That behavior in general is reasonable, but in such a generic function like Datum::add, diverging from PG is surprising behavior, and a potential footgun waiting to go off. I think instead we should have the aggregate functions be responsible for handling NULLs, and mirror PG in this more general case.

However, I opted not to make that change in this PR, as I'm not confident we have test coverage for this case, where a cross shard query returns results from one query and NULL from another. As I refactor this code further, I intend to make that change down the line.

[codecov]

Codecov Report

❌ Patch coverage is 45.88235% with 46 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pgdog-postgres-types/src/datum.rs	44.44%	40 Missing ⚠️
pgdog-postgres-types/src/interval.rs	0.00%	3 Missing ⚠️
pgdog-postgres-types/src/numeric.rs	0.00%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!

[meskill]

I like the direction to more explicit handling that return Result instead of implicit nulls somewhere.

But, considering the 2 points you've mentioned I do have strong feeling that the current add implementation is aggregate specific and exactly that's why the types coercion is not relevant here and nulls are handled like this. I'd actually suggest to move this implementation to the aggregate module and use it only there without affecting the base logic for Datum type (and I think we don't have a case for Datum math outside aggregate?).

Also, please note that the things we are discussing are also related to Ord, Eq implementations on Datum.

P.S.: do we need to encode NULL as the part of Datum or Option could be better fit in some cases?

[sgrif]

My personal preference would be Option, but NULL as part of Datum matches PG's semantics most closely.

[sgrif]

I agree with you that Datum should not implement Ord or Eq, I'm currently working on that in another branch

…

On Fri, Jun 5, 2026, 2:56 PM Kiryl Mialeshka ***@***.***> wrote: *meskill* left a comment (pgdogdev/pgdog#1039) <#1039 (comment)> I like the direction to more explicit handling that return Result instead of implicit nulls somewhere. But, considering the 2 points you've mentioned I do have strong feeling that the current add implementation is aggregate specific and exactly that's why the types coercion is not relevant here and nulls are handled like this. I'd actually suggest to move this implementation to the aggregate module and use it only there without affecting the base logic for Datum type (and I think we don't have a case for Datum math outside aggregate?). Also, please note that the things we are discussing are also related to Ord, Eq implementations on Datum. P.S.: do we need to encode NULL as the part of Datum or Option could be better fit in some cases? — Reply to this email directly, view it on GitHub <#1039?email_source=notifications&email_token=AALVMKYKQ5KTEL4VSKYMSA346MXXNA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINRTGU2DMMBVGAZKM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-4635460502>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALVMK6YMMZXUSWZ67XFNF346MXXNAVCNFSM6AAAAACZ4G7QI6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DMMZVGQ3DANJQGI> . Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS <https://github.com/notifications/mobile/ios/AALVMK5PKUSAFFKCEOLXB4D46MXXNA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINRTGU2DMMBVGAZKM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG> and Android <https://github.com/notifications/mobile/android/AALVMK3H77FLSPPUBDKWDYD46MXXNA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINRTGU2DMMBVGAZKM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>. Download it today! You are receiving this because you authored the thread.Message ID: ***@***.***>