fix: Filter splitting and rendering for related types #1541
Conversation
jsimnz
added
bug
jsimnz
changed the title
jsimnz
changed the title
Codecov Report
@@ Coverage Diff @@
## develop #1541 +/- ##
===========================================
+ Coverage 72.10% 72.26% +0.15%
===========================================
Files 185 185
Lines 18295 18374 +79
===========================================
+ Hits 13192 13278 +86
+ Misses 4060 4054 -6
+ Partials 1043 1042 -1
|
| v := v.([]any) | ||
| logicMapEntries := make([]any, len(v)) | ||
| for i, item := range v { | ||
| itemMap := item.(map[connor.FilterKey]any) |
There was a problem hiding this comment.
question: Can this ever not be a map[connor.FilterKey]any type?
There was a problem hiding this comment.
I did have a whole type checks for all the type casting in this function. But there really isn't a way for them, to not be the respective types, unless there is a notable bug, in which case it should be caught much higher in the call stack.
| }, | ||
|
|
||
| // @shahzadlone: Do I need this? | ||
| // ExpectedPatterns: []dataMap{basicPattern}, |
There was a problem hiding this comment.
answer: Yes, you should assert that the order of the plan graph nodes is what we expect.
| }, | ||
| }, | ||
|
|
||
| // @shahzadlone: Do I need this? |
There was a problem hiding this comment.
answer: Yes, you should assert that the order of the plan graph nodes is what we expect.
Good catch, will do |
jsimnz
force-pushed
the
jsimnz/fix/I1539-I1540-filter-splitting
branch
from
4312f46 to
b58be38
Compare
There was a problem hiding this comment.
Left some todos. In addition I would add the explain tests you are adding to a new file as the test files are split according to different patterns, the patterns of tests you added are more similar to the type join tests, so make a file called ".../default/type_join_with_filter_test.go" and add these two there.
All the todos and the above suggestion have been done here in this commit, feel free to copy: shahzadlone@e57dff5
jsimnz
force-pushed
the
jsimnz/fix/I1539-I1540-filter-splitting
branch
from
e461607 to
38eb4ed
Compare
| childExplainGraph[filterLabel] = nil | ||
| } else { | ||
| childExplainGraph[filterLabel] = c.Filter.ExternalConditions | ||
| childExplainGraph[filterLabel] = c.Filter.ToMap(n.documentMapping) |
There was a problem hiding this comment.
question: Does this need a new test as well?
…1541) Fixes the filter splitting of related types issue. This issue was also directly related to the rendering of these related filters within the explain requests, so this PR covers both. fixes: - Filter splitting - New filter rendering utilities - Aggregate/group explain filter rendering
Relevant issue(s)
Resolves #1539, #1540
Description
Fixes the filter splitting of related types issue. This issue was also directly related to the rendering of these related filters within the explain requests, so this PR covers both.
Tasks
How has this been tested?
Manually, CI
Extensive manual testing with varied queries, extended integration suite with both query and explain tests. Lastly verified against a complex partner query that caused the investigation into the bug originally.
Specify the platform(s) on which this was tested: