-
Notifications
You must be signed in to change notification settings - Fork 25
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
Hybrid query #1468
Hybrid query #1468
Conversation
9c35083
to
5a31178
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1468 +/- ##
==========================================
- Coverage 93.11% 93.03% -0.09%
==========================================
Files 236 236
Lines 42853 42892 +39
==========================================
Hits 39903 39903
- Misses 2950 2989 +39 ☔ View full report in Codecov by Sentry. |
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 ok to me, but hard to tell w/o actually running this code
@akoshelev agreed. this will require an integration test, yea? @tyurek is making progress on #1469, which should then allow us to write an integration test for this. |
ipa-core/src/query/runner/hybrid.rs
Outdated
let ctx = ShardedMaliciousContext::new_with_gate(prss, gateway, gate, sharded); | ||
|
||
// TODO: load this from individual reports | ||
let hybrid_info = |
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.
@tyurek is it something you would need to fix along with the AAD change?
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.
actually this should be fixed. I'll find time to rebase this evening
5a31178
to
4bff1d6
Compare
this is included in #1479, so closing this one. |
Adding one more layer of plumbing to actually running a hybrid query.
This is an early draft to get some input on the direction. We likely won't want to merge this until @tyurek's work to load the
HybridInfo
from the reports directly is complete.