Rewrite type handler lookup #4900
Conversation
vonzshik
force-pushed
the
4481-rewrite-type-handler-lookup
branch
from
a29fbd0 to
f905188
Compare
vonzshik
force-pushed
the
4481-rewrite-type-handler-lookup
branch
from
f905188 to
98a4e9a
Compare
vonzshik
force-pushed
the
4481-rewrite-type-handler-lookup
branch
from
98a4e9a to
47fa852
Compare
| _handlersByOID[oid] = handler; | ||
| return true; | ||
| } | ||
| if ((handler = ResolveByPostgresType(pgType)) is not null) |
There was a problem hiding this comment.
I don't remember all the nuances any more, but...
Does this intentionally change our behavior for resolving by Name vs. FullName? Previously we tried twice (FullName, then Name), whereas now we just call ResolveByPostgresType with pgType; this caches by FullName, but the new resolvers' ResolveByPostgresType looks at Name (is that discrepancy intentional too?).
There was a problem hiding this comment.
Yeah, that's intentional. The reason we were doing it twice it because some resolvers (like builtin) don't care about FullName. I'm pretty sure none of out resolvers actually need FullName.
The point with ResolveByPostgresType is that we pass the type to the resolver, which will decide whether it can handle this type or not. To make it a bit easier, by default resolver's ResolveByPostgresType just calls ResolveByDataTypeName with a short name, and users have an option to override this.
As for the caching, we cache by FullName because it's unique (technically more correct is to cache by PgType, but this looks like an overkill).
There was a problem hiding this comment.
I'm pretty sure none of out resolvers actually need FullName.
Yeah, that makes sense. The only types which need a full name (i.e. in non-pg_catalog schemas) are extensions, and those can be in any schema, based on how the user adds the extension.
|
|
||
| if (!DatabaseInfo.ByOID.TryGetValue(oid, out var pgType)) | ||
| return false; | ||
| return TryResolveLong(oid, out handler); |
There was a problem hiding this comment.
Good optimization - we can do it in ResolveByClrType as well (maybe other methods?)
There was a problem hiding this comment.
Yeah, only if ResolveByClrType, everything else is already done.
2 participants
Closes #4481
Closes #4429
The main reason for this PR is to unblock #4899. In that PR ranges are resolved by a separate resolver, which isn't compatible with NodaTime resolver. Specifically, while reading a range of timestamptz right now we first attempt to find the type by it's full name (which fails), then by it's name (at which point NodaTime's resolver gives the handler back). After, if everything else fails, we have a special logic for complex types (including ranges). In #4899 range's resolver finds the type by it's full name and because it's a range type it gives the range handler back. With this PR we'll be able to immediately check in NodaTime's resolver by name.