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.

Sign up for GitHub

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

Jump to bottom

Add IN and NIN support #16

Closed
wants to merge 7 commits into from
Closed

Add IN and NIN support #16

wants to merge 7 commits into from

Conversation

ashtonian
Copy link
Contributor

@ashtonian ashtonian commented Jul 17, 2019 •
edited
Loading

First attempt #10

The equality fn for the test doesn't like comparing slices because its deep, not sure what we want to do there.

Looking at integration testing next.

@ashtonian
Copy link
Contributor Author

resolves #8

@ashtonian
Copy link
Contributor Author

due to how args are handled by sql.query, this needs to be changed a bit.

Currently assuming the parser receives valid rql it produces something like Select * FROM table WHERE id > ? and name IN (?), and args would contain a slice as an argument [5, [1,2]]. .query wants something more like Select * FROM table WHERE id > ? and name IN (?, ?) and args to be flat [5,1,2].

related: https://stackoverflow.com/questions/20271123/how-to-execute-an-in-lookup-in-sql-using-golang/32837541

ashtonian
ashtonian commented Jul 29, 2019
View reviewed changes
rql_test.go
@@ -898,7 +923,7 @@ func equalExp(e1, e2 string) bool {

func split(e string) []string {
var s []string
for len(e) > 0 {
for len(e) > 0 { // TODO: stuck in loop when parsing (?,?,?) instead of ?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently breaks

ashtonian
ashtonian commented Jul 29, 2019
View reviewed changes
rql.go
}
must(validateFn(opVal), "invalid datatype or format for field %q", f.Name)

if opName == p.op(NIN) || opName == p.op(IN) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a better way to do this ?

ashtonian
ashtonian commented Jul 29, 2019
View reviewed changes
Copy link
Contributor Author

@ashtonian ashtonian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please look at these areas

@ashtonian
Copy link
Contributor Author

updated to support multiple ? placeholders, added working integration tests, currently could use some clean up and the tests are broken, not sure why.

@jonathanroze
Copy link

Any news about this feature ? Thanks

@c84c
Copy link

c84c commented Jan 25, 2023

updated to support multiple ? placeholders, added working integration tests, currently could use some clean up and the tests are broken, not sure why.

If you give me access to your fork, I can help you

@ashtonian
Copy link
Contributor Author

I'll take another stab at this pending review for #49

@ashtonian ashtonian closed this Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ashtonian @jonathanroze @c84c