-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Extend ApproxPercentileResultVerifier for window fuzzer #10367
Conversation
✅ Deploy Preview for meta-velox canceled.
|
This pull request was exported from Phabricator. Differential Revision: D59257657 |
This pull request was exported from Phabricator. Differential Revision: D59257657 |
4e09be4
to
3de0b13
Compare
…bator#10367) Summary: Pull Request resolved: facebookincubator#10367 Differential Revision: D59257657
This pull request was exported from Phabricator. Differential Revision: D59257657 |
…bator#10367) Summary: Pull Request resolved: facebookincubator#10367 Differential Revision: D59257657
3de0b13
to
48450bc
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.
LGTM just a few comments, could you also address the internal lint issues?
@@ -106,6 +134,13 @@ class ApproxPercentileResultVerifier : public ResultVerifier { | |||
return false; | |||
} | |||
} | |||
if (verifyWindow_ && numNonNull != allRanges_->size()) { |
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.
do you need the condition on verifyWindow_? I think this would have caught the bug I fixed in my previous change to this class where we were losing the first percentile
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.
Yes, we do need verifyWindow_ here because for the verification for aggregation fuzzer, the number of non-null rows in result doesn't necessarily match the number of non-null rows in allRanges_, so we cannot compare them.
return "partition by " + folly::join(", ", partitionByKeys); | ||
} | ||
|
||
// For each input row, calculates a map of {value : [order_min, order_max]} as |
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 for adding such a detailed comment!
// c1, | ||
// row_num, | ||
// bucket_element, | ||
// SUM(bucket_weight) AS weight |
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.
why do we need this?
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.
Good catch! I removed this aggregation.
This pull request was exported from Phabricator. Differential Revision: D59257657 |
48450bc
to
7151ca7
Compare
…bator#10367) Summary: Pull Request resolved: facebookincubator#10367 Reviewed By: kevinwilfong Differential Revision: D59257657
This pull request was exported from Phabricator. Differential Revision: D59257657 |
…bator#10367) Summary: Pull Request resolved: facebookincubator#10367 Reviewed By: kevinwilfong Differential Revision: D59257657
7151ca7
to
d850c4f
Compare
This pull request was exported from Phabricator. Differential Revision: D59257657 |
…bator#10367) Summary: Pull Request resolved: facebookincubator#10367 Reviewed By: kevinwilfong Differential Revision: D59257657
d850c4f
to
d2cebc4
Compare
…bator#10367) Summary: Pull Request resolved: facebookincubator#10367 Reviewed By: kevinwilfong Differential Revision: D59257657
This pull request was exported from Phabricator. Differential Revision: D59257657 |
d2cebc4
to
f85e21d
Compare
This pull request has been merged in 3d19d04. |
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Differential Revision: D59257657