External tables support for HTTP protocol #942
Conversation
|
Thanks @crisismaple. Let me have a look on this today! |
|
@crisismaple I didn't manage to review it yesterday. Hopefully, I will have time today. |
Take your time please~ |
|
@crisismaple could you rebase your PR? |
|
|
just updated |
| @@ -265,7 +267,7 @@ func (h *httpConnect) isBad() bool { | |||
| } | |||
|
|
|||
| func (h *httpConnect) readTimeZone(ctx context.Context) (*time.Location, error) { | |||
| rows, err := h.query(ctx, func(*connect, error) {}, "SELECT timezone()") | |||
| rows, err := h.query(Context(ctx, ignoreExternalTables()), func(*connect, error) {}, "SELECT timezone()") | |||
There was a problem hiding this comment.
Since it's an internal logic for HTTP proto, we should not leak it into Options struct.
Instead, can we reset external in context? Or can we pass implicit bool to query func?
There was a problem hiding this comment.
looks like query is defined in stdConnect interface with a generic signature, modify context from caller side seems a more reasonable way to optimize internal calls for timezone and ping.
conn_http.go
Outdated
| return req, nil | ||
| } | ||
|
|
||
| func (h *httpConnect) prepareRequest(ctx context.Context, query string, options *QueryOptions, headers map[string]string) (*http.Request, error) { | ||
| if options == nil || options.ignoreExternalTables || len(options.external) == 0 { |
There was a problem hiding this comment.
| if options == nil || options.ignoreExternalTables || len(options.external) == 0 { | |
| if options == nil || len(options.external) == 0 { |
ignoreExternalTables looks redundant in this use case, if we properly pass the context here.
There was a problem hiding this comment.
just pushed an update to remove ignoreExternalTables in options
| @@ -148,6 +148,13 @@ func WithUserLocation(location *time.Location) QueryOption { | |||
| } | |||
| } | |||
|
|
|||
| func ignoreExternalTables() QueryOption { | |||
| return func(o *QueryOptions) error { | |||
| o.external = nil | |||
There was a problem hiding this comment.
force set external to nil here
jkaflik
changed the title
Summary
use mutipart http request to send both external tables and query
Checklist