Support comparing to another fields #46
Conversation
|
Using our TypeAdapterFactory to add a third type and extending our class picking logic in there to look at the type of values also to pick the right class on deserialization seems like a slightly cleaner way to go. |
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return "{" + super.toString() + ", " + "field: " + field + ", " + "values: " + values + "}"; | ||
| } | ||
|
|
||
| private static Pattern compile(String value) { | ||
| /** | ||
| * Pattern compiler Method. |
There was a problem hiding this comment.
"Common method to compile patterns"? Might become private based on another review comment
| @@ -0,0 +1,73 @@ | |||
| /* | |||
| * Copyright 2017, Yahoo Inc. | |||
| CAST | ||
| } | ||
| @Expose | ||
| Kind kind; |
| this.field = stringFilterClause.field; | ||
| this.patterns = stringFilterClause.patterns; | ||
| List<String> stringValues = stringFilterClause.getValues(); | ||
| if (stringValues != null) { |
There was a problem hiding this comment.
This can't be null because it is a condition in your parser
There was a problem hiding this comment.
In our tests, we new a stringFilterClause and do not set values field.
| @Override | ||
| public void configure(BulletConfig configuration) { | ||
| if (operation == REGEX_LIKE) { | ||
| patterns = values.stream().map(v -> FilterClause.compile(v.getValue())).filter(Objects::nonNull).collect(Collectors.toList()); |
There was a problem hiding this comment.
If you abstract how to get a string from a filter clause, you can leave configure defined in the FilterClause and not duplicate it.
Something like:
if (operation == REGEX_LIKE) {
patterns = values.stream().map(v -> FilterClause.compile(this.getValue(v)).filter(Objects::nonNull).collect(Collectors.toList());
}
StringFilterClause: getValue(String type) -> return type
ObjectFilterClause: getValue(Value type) -> return type.getValue()
| "'fields' : " + makeGroupFields(fields) + ", " + | ||
| "'attributes' : {" + | ||
| "'operations' : [" + | ||
| operations.stream().map(QueryUtils::makeGroupOperation).reduce((a, b) -> a + " , " + b).orElse("") + |
There was a problem hiding this comment.
Redo the indentation please?
| "'fields' : " + makeGroupFields(fields) + ", " + | ||
| "'size' : " + size + ", " + | ||
| "'attributes' : " + makeMap(attributes) + | ||
| "'type' : '" + getTypeFor(operation) + "', " + |
There was a problem hiding this comment.
These were being lined up with the second ". Let's keep it that way for everything?
| @@ -0,0 +1,33 @@ | |||
| /* | |||
| * Copyright 2017, Yahoo Inc. | |||
| @@ -490,6 +491,47 @@ public void testBasicWindowMaximumEmittedWithNonMatchingRecords() { | |||
| Assert.assertNull(querier.getData()); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testLogicFilterNot() { | |||
There was a problem hiding this comment.
Let's add a test for what happens when you mix and match ObjectFilterClause and StringFilterClause (both cases when first is String or Object). We parse by only looking at the first value. Are the other filters ignored? Are there RunTimeExceptions?
There was a problem hiding this comment.
They are RunTimeExceptions. Added 2 tests.
| @@ -591,4 +595,14 @@ public void testNested() { | |||
| Assert.assertFalse(FilterOperations.perform(recordF, clause)); | |||
| Assert.assertTrue(FilterOperations.perform(recordG, clause)); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testCompareToFields() { | |||
There was a problem hiding this comment.
Can we add a test for what happens when an ObjectFilterClause is being tested and the cast cannot be done (the RunTimeException is thrown)?
There was a problem hiding this comment.
It will be ignored. Added a test.
Partially addresses #36.