Skip to content

Conversation

@roji
Copy link
Member

@roji roji commented Jun 22, 2023

Based on @vonzshik's work in #5009. Notes:

  • Split the "unsupported" stuff out of the built-in resolver, and put it in a new dedicated resolver that's always at the very end.
  • Each type handler resolver factory now determines where it inserts itself in the resolver chain. The range and array resolvers insert themselves after built-in (byte[]/string must be resolved by built-in), but before the last unsupported handler; range must come before array (because multiranges have precedence over arrays of ranges).
  • Lots of test cleanup, don't be too alarmed by the PR size :)

Closes #4988

@roji roji requested review from NinoFloris and vonzshik June 22, 2023 13:29
/// <summary>
/// Sets up mappings for the arrays.
/// </summary>
public NpgsqlSlimDataSourceBuilder EnableArrays()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably think about this in the context of my rework, as we really should design for trimability here.

The difficulty lies with the globalness of this method (it knows nothing about individual resolvers) while having to opt-in per resolver to get static trimability of not including all the array mappings.

Maybe we can do something clever with an interface on resolvers that support arrays to be able to pull out the WithArray resolver type (and replace the resolver in the list for the new 'WithArray' version).

At that point - as far as I understand - all implementations of that interface will get rooted, basically global opt-in with extra steps.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, probably the same thing applies for ranges etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ranges are a bit different in the sense that they're not an automatically derived type. A range is its own type (and you can have an array of that range again). So you're right in the sense that we probably should rename EnableRanges to EnableBuiltinRanges given those are the ranges we'll start supporting.

Multiranges is probably the best analog to arrays as it's similarly automatically there, just only for range types. Those probably deserve the same cross-cutting opt-in treatment as arrays should ideally have :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, built-in ranges are a very constrained set, whereas there's an array for every base type (and more), that's true.

Closes npgsql#4988

Co-authored-by: Nikita Kazmin <vonzshik@gmail.com>
@vonzshik
Copy link
Contributor

@roji do you want me to take a look or we're putting this on break until @NinoFloris's changes are merged?

@roji
Copy link
Member Author

roji commented Jul 25, 2023

Definitely not - this is superceded by @NinoFloris's work... However, there's some test work I did in this PR which we should still merge (some I already brought into Nino's PR, but not sure if everything). I'll do that soon.

@roji roji marked this pull request as draft July 25, 2023 10:45
@roji roji changed the title Make arrays opt-in Make arrays opt-in (superceded) Jul 25, 2023
@NinoFloris NinoFloris mentioned this pull request Aug 15, 2023
8 tasks
@NinoFloris NinoFloris added this to the 8.0.0 milestone Sep 25, 2023
@Brar Brar removed this from the 8.0.0 milestone Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make arrays opt-in on NpgsqlSlimDataSourceBuilder

4 participants