-
Notifications
You must be signed in to change notification settings - Fork 44
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
fix: Resolve the extra typeIndexJoin
s for _avg
aggregate
#774
Conversation
06c8f10
to
6696f43
Compare
Codecov Report
@@ Coverage Diff @@
## develop #774 +/- ##
===========================================
+ Coverage 59.17% 59.27% +0.09%
===========================================
Files 153 153
Lines 16994 17035 +41
===========================================
+ Hits 10057 10098 +41
Misses 6020 6020
Partials 917 917
|
2076e5d
to
7f66c60
Compare
@@ -530,176 +438,6 @@ func TestExplainAverageQueryOnMultipleJoinedFieldWithFilter(t *testing.T) { | |||
}, | |||
}, | |||
}, | |||
{ |
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.
praise: Nice 😆
7f66c60
to
c39a42b
Compare
c39a42b
to
ff3a2e4
Compare
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.
Looks much better than cloning and derefing and is easy to read, but added a couple of todos/question for you
@@ -9,4 +9,6 @@ type FilterKey interface { | |||
// GetOperatorOrDefault returns either the operator that corresponds | |||
// to this key, or the given default. | |||
GetOperatorOrDefault(defaultOp string) string | |||
// Equal returns true if other is structurally equal, otherwise returns false. | |||
Equal(other FilterKey) 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.
Nice :) Was hoping you would do this
ff3a2e4
to
9c7a386
Compare
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.
Approved, looks good, but 3 more comments that should preferably be addressed first (particularly the one RE why you keep looping when you already know that it is not equal)
Maybe there are some unpending comments, I only see 2 new comments + this one I am replying to. Which comment are you referring to here: 'RE why you keep looping when you already know that it is not equal'? |
EDIT: I think I know which one you are talking about left a follow-up reply. |
9c7a386
to
2371b9a
Compare
2371b9a
to
62ad1ec
Compare
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.
Looking good. Just a few comments :)
62ad1ec
to
e77aa72
Compare
a dereferenced version of the filter conditions and also get rid of reflect.DeepEqual() comparison. - Adhere to code review.
e77aa72
to
8f19fc5
Compare
…etwork#774) - Resolves sourcenetwork#640 - Description: * Add limit check to `tryGetTarget` function. * Fix the equality check with the filter to find the host. * Update the previously documented tests that had incorrect behavior.
Resolves #640
Description
tryGetTarget
function.More Info on the bug:
The host was not being found and kept getting added incorrectly, this is because
reflect.DeepEqual
Fails when you have a map where the key is pointer.In this case it was slightly more hidden because the key was an interface with an underlying pointer. Here is a more accurate example on what was happening (the last case represents what was happening):
Can run the above here: https://go.dev/play/p/bBvUubM8u_N