-
Notifications
You must be signed in to change notification settings - Fork 812
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
docstore/all - add support for boolean filter #3446
Conversation
Hey @vangent, would it be possible to have a review for my CR, please? |
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.
Sorry, I didn't have access to AWS for a while so I wasn't able to run the tests.
I will have to copy this PR into a new one in order to update the goldens for you, but will leave comments here so you can iterate.
@@ -42,6 +42,7 @@ aws dynamodb create-table \ | |||
AttributeName=Player,AttributeType=S \ | |||
AttributeName=Score,AttributeType=N \ | |||
AttributeName=Time,AttributeType=S \ | |||
AttributeName=WithGlitch,AttributeType=BOOL \ |
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.
This doesn't work, the only possible values are S, N, B. I am not sure what to use for a BOOL value....
Although, the test seems to pass without this so maybe it's not necessary?
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.
Yes you are correct, I have removed it
@@ -1485,6 +1486,11 @@ func testGetQuery(t *testing.T, _ Harness, coll *docstore.Collection) { | |||
q: coll.Query().Where("Player", "not-in", []string{"pat", "billie"}), | |||
want: func(h *HighScore) bool { return h.Player != "pat" && h.Player != "billie" }, | |||
}, | |||
{ | |||
name: "WithGlitch", |
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.
Can you also add tests for "in" and "not-in"?
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.
Yes it's done
…s attribute name when creating dynamodb table
I have cloned this PR to #3464 and added the AWS and GCP golden test file updates. |
Thanks for the contribution! |
This PR updates Docstore to allow boolean filters in the queries, as it's supported by all the different implementations.
This isn't yet merge-able as the replay data needs to be re-generated, and I don't have the permission to do it.
I have tested it with Firestore, the local version of MongoDB but haven't tested it with AWS or Azure as I don't have an account.