-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Move Covariance (Population) covar_pop to be a User Defined Aggregate Function #10418
Conversation
@@ -319,281 +225,3 @@ impl Accumulator for CovarianceAccumulator { | |||
std::mem::size_of_val(self) | |||
} | |||
} | |||
|
|||
#[cfg(test)] | |||
mod tests { |
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.
I was thinking whether we should move these tests into the new files rather than deleting them. 🤔
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.
you can move it to aggregate.slt like what in #10384 does
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.
you can create another PR for moving test
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.
Got it.
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.
Thank you @yyy1000 and @jayzhan211 -- I agree once we port over the tests this PR should be good to go 🙏
Would you prefer moving test in a separate PR or in this PR? I'm good either way. :) |
If the test can be verified one by one less than 5min, than it is fine for me to see them in one PR. |
let state2 = get_accum_scalar_values_as_arrays(accum2.as_mut())?; | ||
accum1.merge_batch(&state2)?; | ||
accum1.evaluate() |
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.
I added all unit tests to sqllogictest excluding covariance_f64_merge_1
and covariance_f64_merge_2
Don't know how to mock these merge operations. 🥲
)); | ||
|
||
let actual = merge(&batch1, &batch2, agg1, agg2)?; | ||
assert!(actual == ScalarValue::from(0.6666666666666666)); |
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.
we just need to test with multi column
covariance(a, b)
, where a is [1, 2,3]. b is [4,5,6]
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.
Got it, thanks!
I checked these two test case has been covered in covariance_f64_1
and covariance_f64_6
so all the tests are ported.
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.
👍
Thanks @yyy1000 and @jayzhan211 |
… Function (apache#10418) * move covariance * add sqllogictest
Which issue does this PR close?
Closes #10389 .
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?