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

Function Overload Rework #133

Merged
merged 2 commits into from
Feb 29, 2024
Merged

Conversation

Hydrocharged
Copy link
Collaborator

@Hydrocharged Hydrocharged commented Feb 20, 2024

What this adds:

  • Type resolution for all extended (Doltgres) types
  • Proper type casting between types (extendable to support user-defined conversion)
  • Additional Types

The previous function overload framework only worked in a very limited capacity. This PR intends to remove those limitations, allowing for all current (and future) Postgres functions to be defined. In general, this is also designed to support user-defined functionality, or at least designed in such a way that it will be relatively easy to fully implement it later on.

Related PR: dolthub/dolt#7537

@Hydrocharged
Copy link
Collaborator Author

Hydrocharged commented Feb 20, 2024

Build currently fails as I need to finish converting all of the functions over to the new framework. Also need to add more types. Neither of those will take long.

However, I'm still trying to find a good way to represent type conversions. varchar(255) and varchar(256) currently compare as different types, which is correct. However, they're the same base type, and I need to figure out a good solution for that which will work with conversions (i.e. map storage, etc.). In GMS we use query.Type for this purpose, which will work fine for the default types, but won't work for user-defined types.

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.

What's here looks fine, just some comments about the organization. Approving without passing tests assuming that changes will be minor, ping me if that's not the case

server/functions/zinternal_compiled_function.go Outdated Show resolved Hide resolved
server/functions/zinternal_overload_deduction.go Outdated Show resolved Hide resolved
server/types/float32.go Outdated Show resolved Hide resolved
server/types/serialization.go Outdated Show resolved Hide resolved
@Hydrocharged Hydrocharged force-pushed the daylon/function-overloads-redone branch from ee2b6e4 to de0f152 Compare February 26, 2024 15:57
@Hydrocharged Hydrocharged marked this pull request as ready for review February 26, 2024 15:57
@Hydrocharged
Copy link
Collaborator Author

The replication tests will fail, but for the most part this is reviewable.

@Hydrocharged Hydrocharged force-pushed the daylon/function-overloads-redone branch 3 times, most recently from 6136021 to 9584086 Compare February 28, 2024 12:59
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

My main comment about this is that we need engine (server) tests for all the functions you defined. Make that a fast-follow to this.

server/ast/resolvable_type_reference.go Show resolved Hide resolved
server/connection_handler.go Show resolved Hide resolved
server/expression/addition.go Show resolved Hide resolved
server/expression/cast.go Outdated Show resolved Hide resolved
server/expression/cast.go Outdated Show resolved Hide resolved
server/functions/framework/cast.go Show resolved Hide resolved
server/functions/framework/catalog.go Outdated Show resolved Hide resolved
server/functions/framework/compiled_function.go Outdated Show resolved Hide resolved
@Hydrocharged Hydrocharged force-pushed the daylon/function-overloads-redone branch from 9584086 to eaf588e Compare February 29, 2024 11:24
@Hydrocharged Hydrocharged force-pushed the daylon/function-overloads-redone branch from eaf588e to 6b2e975 Compare February 29, 2024 11:28
@Hydrocharged Hydrocharged merged commit 4798d63 into main Feb 29, 2024
8 checks passed
@Hydrocharged Hydrocharged deleted the daylon/function-overloads-redone branch February 29, 2024 12:29
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.

None yet

2 participants