-
Notifications
You must be signed in to change notification settings - Fork 447
Feat/require target state keys to fit into type stable key #1625
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
base: v1
Are you sure you want to change the base?
Feat/require target state keys to fit into type stable key #1625
Conversation
|
thanks a lot @Geoff-Robin will get back on this shortly! thanks a lot for your contributions! |
| impl crate::engine::profile::Persist for StableKey { | ||
| fn to_bytes(&self) -> Result<bytes::Bytes> { | ||
| let buf = storekey::encode_vec(self) | ||
| .map_err(|e| internal_error!("Failed to encode StableKey: {e}"))?; | ||
| Ok(bytes::Bytes::from(buf)) | ||
| } | ||
|
|
||
| fn from_bytes(data: &[u8]) -> Result<Self> { | ||
| storekey::decode(std::io::Cursor::new(data)) | ||
| .map_err(|e| internal_error!("Failed to decode StableKey: {e}")) | ||
| } | ||
| } | ||
|
|
||
| impl crate::engine::profile::StableFingerprint for StableKey { | ||
| fn stable_fingerprint(&self) -> utils::fingerprint::Fingerprint { | ||
| match self { | ||
| StableKey::Fingerprint(fp) => *fp, | ||
| _ => { | ||
| let mut fingerprinter = utils::fingerprint::Fingerprinter::default(); | ||
| let bytes = crate::engine::profile::Persist::to_bytes(self) | ||
| .expect("StableKey encoding for fingerprint"); | ||
| fingerprinter.write_raw_bytes(&bytes); | ||
| fingerprinter.into_fingerprint() | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
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 don't need to add these. StableKey itself is already serializable and deserializable.
Since StableKey is a type defined in the core crate, we can remove TargetStateKey and directly use StableKey. So we can directly serialize/deserialize it in the engine code.
rust/py/src/stable_path.rs
Outdated
|
|
||
| use cocoindex_core::state::stable_path::{StableKey, StablePath}; | ||
|
|
||
| impl PyStableKey { |
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 think we want to implement IntoPyObject for PyStableKey instead (which matches FromPyObject below)
| #[pyfunction] | ||
| pub fn declare_target_state<'py>( | ||
| py: Python<'py>, | ||
| _py: Python<'py>, |
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 no longer need this argument.
| ): | ||
| if isinstance(key, tuple) and not isinstance(key, _TableKey): | ||
| key = _TableKey(*key) | ||
|
|
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.
The type annotation key: _TableKey is not right now. I think we want to use key: StableKey instead, since it's the actual type being passed around. We can remove KeyT / KeyT_contra here, and directly use StableKey.
This will also force each reconcile() implementation explicitly convert the type, so implementers won't forget.
Key Changes
Removed
PyKeyPyKeystruct and its serialization logic were entirely deleted fromrust/py/src/value.rs.Adoption of
StableKeyStableKeyinstead.Rust Implementation Update
TargetStateinrust/py/src/target_state.rsto interact with this new key type.rust/core/src/state/stable_path.rsandrust/py/src/stable_path.rsto bridge the new key references.Resolves this particular issue