-
Notifications
You must be signed in to change notification settings - Fork 2
feat: update sticky #38
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
Conversation
a9676ef
to
9256078
Compare
9256078
to
56735bc
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.
I left a few comments, but didn't really check the materialization logic, it's too dense to follow, but I trust it's doing the right thing! 🙂
use core::marker::PhantomData; | ||
use fastmurmur3::murmur3_x64_128; | ||
use std::collections::{HashMap, HashSet}; | ||
use std::collections::{BTreeMap, HashMap, HashSet}; |
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 a BTreeMap? Do we still have protos configured to use btrees?
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.
yeah .. seems like it.
|
||
message MaterializationContext { | ||
map<string, MaterializationInfo> unit_materialization_info = 1; | ||
message MaterializationMap { |
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 changing name to Map instead of Context?
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.
because it's a map and just thought to remove an unnecessary nested object , and that's how it works also in shadow resolver and flags resolver. so used the same type.
|
||
// Context about the materialization required for the resolve | ||
MaterializationContext materialization_context = 7; | ||
map<string, MaterializationMap> materializations_per_unit = 2; |
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.
Isn't it a problem that we can't differentiate between an empty map and not set? I.e. how does the resolver know if we have fetched everything, but it was empty OR we haven't fetched?
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 will return the default object for materialization info, and set unit_in_info to false.
No description provided.