-
Notifications
You must be signed in to change notification settings - Fork 13
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
feat: SQL query with named parameters #56
Conversation
d8fac13
to
5cab7e8
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #56 +/- ##
==========================================
- Coverage 83.31% 83.09% -0.22%
==========================================
Files 11 12 +1
Lines 935 994 +59
==========================================
+ Hits 779 826 +47
- Misses 128 138 +10
- Partials 28 30 +2 ☔ View full report in Codecov by Sentry. |
influxdb3/query.go
Outdated
@@ -87,21 +108,28 @@ func (c *Client) Query(ctx context.Context, query string) (*QueryIterator, error | |||
// Returns: | |||
// - A custom iterator (*QueryIterator) that can also be used to get raw flightsql reader. | |||
// - An error, if any. | |||
// | |||
// Deprecated: use Query with QueryOption options. |
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.
I think the Deprecation message could be more precise. e.g. "Deprecated: use Query with variadic QueryOption options". The deprecated method itself has a similar argument with a pointer to QueryOptions. At first glance it can be a bit confusing.
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.
The message is updated.
influxdb3/write.go
Outdated
@@ -59,7 +61,17 @@ func (c *Client) WritePoints(ctx context.Context, points ...*Point) error { | |||
// | |||
// Returns: | |||
// - An error, if any. | |||
// | |||
// Deprecated: use WritePoints with WriteOption options. |
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.
Here too maybe the message could be more specific. e.g. "Deprecated: use WritePoints with variadic WriteOption options"
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.
The message is updated.
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.
Tried new features and some old in ad hoc tests here https://github.com/bonitoo-io/influxdb3-client-tests/tree/main/go-client/release-0.6.0.
While these covered the core of the golden path, nothing unexpected occurred and results look sound.
Nit pick:
As mentioned at points in review, the message on deprecated methods can be a bit confusing at first glance. Perhaps it could be improved by being more specific.
For example.
- current: "Deprecated: use Query with WithQueryOptions option"
- suggested: "Deprecated: use Query with variadic QueryOptions option"
The messages are updated. |
Closes #55
Proposed Changes
This PR add supports for named parameters in the query API
QueryWithParameters(context.Context, string, QueryParameters, ...QueryOption)
Breaking changes
Changed existing
Query*
andWrite*
methods fromto
where available options are
QueryOption
:WithDatabase
WithQueryType
WriteOption
:WithDatabase
WithPrecision
WithGzipThreshold
WithDefaultTags
Example:
Checklist