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 an extension point for custom function providers #563
Conversation
b4fc5e1
to
1c9fb6c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Now that we've extracted an interface for Catalog
I wonder about having DatabaseProvider
as a separate concept. Feels like we could combine these
driver/conn.go
Outdated
@@ -23,17 +23,14 @@ import ( | |||
|
|||
// Conn is a connection to a database. | |||
type Conn struct { | |||
options Options | |||
options *Options | |||
catalog *catalog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while you're here, type catalog
on line 155 would benefit from having a new name
return func(ctx *sql.Context, e ...sql.Expression) (sql.Expression, error) { | ||
return NewPad(ctx, pType, e...) | ||
} | ||
func NewLeftPad(ctx *sql.Context, e ...sql.Expression) (sql.Expression, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't break the internet Zach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't ever tell me how to live my life
Yeah, I think we're moving in that direction. If anything, the Catalog concept is a caching layer for the analyzer, not a public interface. Just didn't want to go quite that far in one push. |
As part of this, embarked on a major refactor: