Filter equality matchers on the dynamo side#400
Merged
Conversation
jml
approved these changes
Apr 18, 2017
Contributor
jml
left a comment
jml
left a comment
There was a problem hiding this comment.
Code looks OK to me, but I'd like @tomwilkie to check over the logic.
tomwilkie
reviewed
Apr 19, 2017
chunk/schema.go
Outdated
| RangeValueStart []byte | ||
|
|
||
| // Filters for querying | ||
| ValueEq []byte |
Contributor
There was a problem hiding this comment.
I'm not usually picky about names, I promise :-)
...but I would either call this Value, ValueEqual.
tomwilkie
reviewed
Apr 19, 2017
| items := m.tables[*input.TableName].items[hashValue] | ||
|
|
||
| // TODO we should also filter by range value | ||
| // Optional filters |
Contributor
There was a problem hiding this comment.
Thanks for implementing this! I was being lazy.
tomwilkie
reviewed
Apr 19, 2017
chunk/aws_storage_client_test.go
Outdated
| valueFilter []byte | ||
| valueFilterType *string | ||
| ) | ||
| if input.KeyConditions[rangeKey] != nil { |
Contributor
There was a problem hiding this comment.
It might be more idiomatic to do:
if c, ok := input.KeyConditions[rangeKey]; ok {
rangeValueFilter = c.AttributeValueList[0].B
rangeValueFilterType = *c.ComparisonOperator
}
tomwilkie
reviewed
Apr 19, 2017
chunk/aws_storage_client_test.go
Outdated
| items := m.tables[*input.TableName].items[hashValue] | ||
| for _, item := range items { | ||
| rangeValue := item[rangeKey].B | ||
| if rangeValueFilterType == "GE" && bytes.Compare(rangeValue, rangeValueFilter) < 0 { |
Contributor
There was a problem hiding this comment.
pls use rangeValueFilterType == dynamodb.ComparisonOperatorGe
tomwilkie
reviewed
Apr 19, 2017
chunk/aws_storage_client_test.go
Outdated
| if rangeValueFilterType == "GE" && bytes.Compare(rangeValue, rangeValueFilter) < 0 { | ||
| continue | ||
| } | ||
| if rangeValueFilterType == "BEGINS_WITH" && !bytes.HasPrefix(rangeValue, rangeValueFilter) { |
tomwilkie
reviewed
Apr 19, 2017
chunk/aws_storage_client_test.go
Outdated
|
|
||
| if item[valueKey] != nil { | ||
| value := item[valueKey].B | ||
| if valueFilter != nil && *valueFilterType == "EQ" && !bytes.Equal(value, valueFilter) { |
Contributor
|
Just a bunch of nits, LGTM otherwise. |
Contributor
|
@aaron7 Feel free to merge this if you've addressed all of @tomwilkie's points. |
aaron7
added a commit
that referenced
this pull request
Apr 19, 2017
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #398
IndexEntrywhich is then used to add the filter condition to dynamo if it exists