-
Notifications
You must be signed in to change notification settings - Fork 0
feat: support dialect-aware query conditions #180
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #180 +/- ##
==========================================
+ Coverage 85.58% 87.04% +1.46%
==========================================
Files 78 87 +9
Lines 3343 3851 +508
==========================================
+ Hits 2861 3352 +491
- Misses 328 345 +17
Partials 154 154 ☔ View full report in Codecov by Sentry. |
7a1a31d to
cebc595
Compare
cebc595 to
6854d81
Compare
shubhammehra4
left a comment
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
588fd0a to
2308aef
Compare
sqlconnect/op/operator.go
Outdated
| // v < left OR v > right | ||
| NotBetween Operator = "notbetween" | ||
| // left >= now() - INTERVAL right | ||
| NbfInterval Operator = "nbfinterval" |
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 key nbfinterval is not clear to me. Can we give it a clearer name?
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.
Open to suggestions! nbf prefix stands for not before.
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.
Instead can we have something like
beforeintervalbounded & beforeintervalunbounded
afterintervalbounded & afterintervalunbounded
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.
Also, we can use camelCase or snake case operators. Will make things much clearer. Especially in case we are using bigger ones.
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.
imo operators should be kept small and case insensitive
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.
what about:
gti --> gt an interval
gtei --> gte an interval
lti --> less than an interval
ltei --> less than an interval
betweeni --> between an interval
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.
Another option would be to keep using gt, gte, lt, lte, between, etc. and allow a special value format for intervals, e.g. "now-1d"
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.
finally, we can just keep ti simple for now and name it inlast
sprksh
left a comment
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.
Went through the code and tests. Looks great.
sqlconnect/op/operator.go
Outdated
| // NOT LIKE | ||
| NotLike Operator = "notlike" | ||
| // left <= v <= right | ||
| Between Operator = "between" |
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.
For Between, Should we think of some way to specify left and right bounded/unbounded conditions.
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.
Also NotBetween
🤖 I have created a release *beep* *boop* --- ## [1.10.0](v1.9.0...v1.10.0) (2024-09-12) ### Features * **databricks:** bump github.com/databricks/databricks-sql-go from 1.6.0 to 1.6.1 ([#174](#174)) ([8cab1fd](8cab1fd)) * **snowflake:** bump github.com/snowflakedb/gosnowflake from 1.10.1 to 1.11.1 ([#171](#171)) ([e64ba77](e64ba77)) * support dialect-aware query conditions ([#180](#180)) ([08906ee](08906ee)) ### Miscellaneous * all warehouses support views in ListTables ([#169](#169)) ([098a378](098a378)) * **deps:** bump github.com/aws/aws-sdk-go-v2/credentials from 1.17.27 to 1.17.32 ([#179](#179)) ([47afb2e](47afb2e)) * **deps:** bump github.com/rudderlabs/rudder-go-kit from 0.36.2 to 0.40.0 ([#178](#178)) ([a4ee52e](a4ee52e)) * **deps:** bump google.golang.org/api from 0.194.0 to 0.196.0 ([#177](#177)) ([aaee20d](aaee20d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
Introducing the following methods.
Using
goquinternally which:int,float,string,time.Time, etc.)')bool_col IS TRUEvsbool_col = 'TRUE'.Linear Ticket
resolves PRO-3401
Security