Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added the ExtendedType interface #2312

Merged
merged 1 commit into from
Feb 7, 2024
Merged

Added the ExtendedType interface #2312

merged 1 commit into from
Feb 7, 2024

Conversation

Hydrocharged
Copy link
Contributor

@Hydrocharged Hydrocharged commented Feb 5, 2024

This adds the ExtendedType interface, which is used within DoltgreSQL to implement PostgreSQL types, as well as in Dolt to properly handle the new type and value serialization.

Related PRs:

Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

LGTM

sql/types/extended.go Show resolved Hide resolved
sql/types/extended.go Outdated Show resolved Hide resolved
sql/planbuilder/ddl.go Outdated Show resolved Hide resolved
sql/types/extended.go Show resolved Hide resolved
sql/types/extended.go Outdated Show resolved Hide resolved
Copy link
Contributor

@max-hoffman max-hoffman left a comment

Choose a reason for hiding this comment

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

One weird thing is how this goes against our current type organizational patterns. There is a world where GMS doesn't know anything about type implementations, and Dolt/Postgres initializes an engine with types alongside storage specific serialization and comparison code. I think that could be better in some ways, because the engine wouldn't have to hop back and forth between the prolly and sql layers as often. Better perf and probably less type indirection. That's kind of what ExtendedType does. On the other hand, all of the other types explicitly separate logical and storage layer details. And that's something that doesn't seem much harder than the existing dolt changes outside of typeinfo and types which are almost deprecated. GMS could have an extended type for behavioral details, and Dolt could have a similar thing for extending serialization patterns for the same ExtendedTypes. For me it's weird to support both patterns at the same time. That plus skipping semantic resolution are my biggest concerns.

sql/planbuilder/scalar.go Outdated Show resolved Hide resolved
sql/types/extended.go Outdated Show resolved Hide resolved
@Hydrocharged Hydrocharged merged commit c0f397a into main Feb 7, 2024
7 checks passed
@Hydrocharged Hydrocharged deleted the daylon/super-type branch February 7, 2024 12:45
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.

3 participants