Flag From Tag dimension #913
Conversation
| if (! (entry.getKey() instanceof FilterOptimizable)) { | ||
| newFilters.put(entry.getKey(), entry.getValue()); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This is a bug.
If Dimension A is processed and optimizes filters into Dimension B, and then Dimension B is processed, this put will cause it's filters to overwrite the filters that were originally dimension A.
Probably the best approach here is to create a List of entries and push values into it and then do a grouping collect at the end.
| @@ -1,4 +1,4 @@ | |||
| // Copyright 2019 Oath Inc. | |||
| // Copyright 2019 Verizon Media Group. | |||
There was a problem hiding this comment.
Sadly we can't use Verizon Media Group yet as copyright. We're still legally Oath.
| apiRequest.getApiFilters() >> new ApiFilters([(resources.d14) : [new ApiFilter(resources.d14, DefaultDimensionField.ID, DefaultFilterOperation.in, ["TRUE_VALUE"] as Collection)] as Set] as Map) | ||
| } | ||
|
|
||
| def "Flag from tag dimension based on lookup dimension serializes to cascade extraction function with tag extraction and transformation in grouping context"() { |
There was a problem hiding this comment.
I would consider parameterizing these expected values a little more so it's easier to fix the tests is there are subtle changes. Extract the formula for the final extraction function perhaps.
| objectMapper.writeValueAsString(druidQuery).contains('{"type":"extraction","dimension":"breed","outputName":"flagFromTagRegisteredLookup","extractionFn":{"type":"cascade","extractionFns":[{"type":"registeredLookup","lookup":"BREED__SPECIES","retainMissingValue":false,"replaceMissingValueWith":"Unknown BREED__SPECIES","injective":false,"optimize":true},{"type":"registeredLookup","lookup":"BREED__OTHER","retainMissingValue":false,"replaceMissingValueWith":"Unknown BREED__OTHER","injective":false,"optimize":true},{"type":"registeredLookup","lookup":"BREED__COLOR","retainMissingValue":false,"replaceMissingValueWith":"Unknown BREED__COLOR","injective":false,"optimize":true},{"type":"regex","expr":"(.+,)*(TAG_VALUE)(,.+)*","index":2,"replaceMissingValue":true,"replaceMissingValueWith":""},{"type":"lookup","lookup":{"type":"map","map":{"TAG_VALUE":"TRUE_VALUE"}},"retainMissingValue":false,"replaceMissingValueWith":"FALSE_VALUE","injective":false,"optimize":false}]}}') | ||
| } | ||
|
|
||
| def "Flag from tag dimension pushes extraction and transformation to innermost query when nesting"() { |
| @@ -65,9 +66,15 @@ class ConjunctionDruidFilterBuilderSpec extends Specification { | |||
| } | |||
|
|
|||
| @Unroll | |||
| def "buildFilters takes a conjunction of clauses, one for each dimension in #dimensions"() { | |||
| def "buildFilters takes a conjunction of clauses, onex for each dimension in #dimensions"() { | |||
| (dim2) : [dim2OutputFilter] as Set | ||
| ] as Map<Dimension, Set<ApiFilter>>) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Add a test where if dimension A optimizes into dimension B that the resulting query has the merge of filters for dimension B. Ensure this happens regardless of the order of the dimensions.
checkstyle-style.xml
Outdated
| @@ -273,7 +273,7 @@ | |||
| <module name="RegexpHeader"> | |||
| <property name="severity" value="error" /> | |||
| <property name="header" | |||
| value="// Copyright 20[0-1][0-9],? (Yahoo Inc.|Oath Inc.)\n// Licensed under the terms of the Apache license. Please see LICENSE.md file distributed with this work for terms\.\n" /> | |||
| value="// Copyright 20[0-1][0-9],? (Yahoo Inc.|Oath Inc.|Verizon Media Group.)\n// Licensed under the terms of the Apache license. Please see LICENSE.md file distributed with this work for terms\.\n" /> | |||
There was a problem hiding this comment.
Revert this, per Gil Yehuda, we can't use this copyright yet.
…of just raw string comparisons
issue: #915