Feat/require target state keys to fit into type stable key by Geoff-Robin · Pull Request #1625 · cocoindex-io/cocoindex

Geoff-Robin · 2026-02-05T15:12:48Z

Key Changes

Removed PyKey
- The custom PyKey struct and its serialization logic were entirely deleted from rust/py/src/value.rs.
- This type previously allowed wrapping arbitrary Python objects as keys by serializing them.
Adoption of StableKey
- The code was updated to use StableKey instead.
- This enforces a stricter, more deterministic type system for keys used in target states, likely improving reliability and cross-connector compatibility.
Rust Implementation Update
- Target State: Updated TargetState in rust/py/src/target_state.rs to interact with this new key type.
- Path Bridging: Added support code in rust/core/src/state/stable_path.rs and rust/py/src/stable_path.rs to bridge the new key references.

Resolves this particular issue

…e_StableKey

badmonster0 · 2026-02-09T02:42:48Z

thanks a lot @Geoff-Robin will get back on this shortly! thanks a lot for your contributions!

georgeh0 · 2026-02-10T05:38:20Z

rust/core/src/state/stable_path.rs

+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()
+            }
+        }
+    }
+}
+


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.

georgeh0 · 2026-02-10T05:41:07Z

rust/py/src/stable_path.rs


 use cocoindex_core::state::stable_path::{StableKey, StablePath};

+impl PyStableKey {


I think we want to implement IntoPyObject for PyStableKey instead (which matches FromPyObject below)

georgeh0 · 2026-02-10T05:41:31Z

rust/py/src/target_state.rs

 #[pyfunction]
 pub fn declare_target_state<'py>(
-    py: Python<'py>,
+    _py: Python<'py>,


We no longer need this argument.

georgeh0 · 2026-02-10T06:33:40Z

python/cocoindex/connectors/lancedb/_target.py

    ):
+        if isinstance(key, tuple) and not isinstance(key, _TableKey):
+            key = _TableKey(*key)
+


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.

@georgeh0 I've tried doing this but I've encountered a data loss bug in SQLite caused by unstable instance-bound sinks and fixed it by using stable global sinks. Is this fine or maybe I could take a different approach
The tests that i started to fail were:

test_sqlite_target.py::test_regular_table_vs_vec0_switch
started failing around line 1232 > assert len(data) == 0

test_sqlite_target.py::test_vec0_virtual_table_basic
start failing around line 660 > assert len(data) == 1

I will be pushing the code for this to get the whole view

a data loss bug in SQLite caused by unstable instance-bound sinks and fixed it by using stable global sinks.

Thanks for trying out. We want to understand what happens here.

Why the instance-bound sinks were unstable? They're instance-bound but they're stable. And I don't know why a key type change will affect the logic there.

As I commented above, I think this PR is changing more than we need: we still want to use Fingerprint as TargetStatePath. I'm not sure if the failed tests are related to some unexpected changes like this. Please fix that and try again if the sqlite tests will work with the previous code (with necessary changes for key types). Thanks!

It seems like using StableKey directly in TargetStatePath caused the errors, I will be reverting back to fingerprint.
This will undo the changes i've made just now

…e_StableKey

georgeh0 · 2026-02-11T17:12:36Z

rust/core/src/engine/profile.rs

-        + Persist
-        + StableFingerprint
-        + 'static;
+    // TargetStateKey is removed in favor of using StableKey directly.


Why do we need to keep this line of comment?

georgeh0 · 2026-02-11T17:14:22Z

rust/core/src/state/target_state_path.rs

+
 #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)]
-pub struct TargetStatePath(Arc<[utils::fingerprint::Fingerprint]>);
+pub struct TargetStatePath(pub Arc<[StableKey]>);


We don't want to change this part. We still want to use fingerprint here. (We only change whatever was TargetStateKey to StableKey, but Fingerprint is a different thing)

For the TargetStatePath, the only thing we want to change is when we create/concat the path with a new key part, i.e. we compute fingerprint from StableKey instead of the old key.

georgeh0 · 2026-02-11T17:18:38Z

rust/py/src/target_state.rs

        .register(
            TargetStatePath::new(
-                utils::fingerprint::Fingerprint::from(&name).into_py_result()?,
+                cocoindex_core::state::stable_path::StableKey::Str(Arc::from(name)),


We don't need to change this. It's OK to still get fingerprint from the name directly.

georgeh0 · 2026-02-11T19:45:04Z

python/cocoindex/connectors/lancedb/_target.py

+            # Should be tuple for row key
+            if not isinstance(key, tuple):
+                # This might happen if single-value PK is passed as scalar
+                key = (key,)
+        key = cast(_RowKey, key)


If the key type is not a expected, raise an exception.

Then we won't need the cast (when the isinstance() pass, it's always a tuple that satisfies _RowKey). cast itself doesn't turn value in an unexpected type to the expected type.

Please update reconcile() for other targets accordingly.

georgeh0 · 2026-02-11T19:46:03Z

python/cocoindex/connectors/sqlite/_target.py

        self._table_name = table_name
        self._table_schema = table_schema
        self._is_virtual_table = is_virtual_table
-        self._sink = coco.TargetActionSink[_RowAction, None].from_fn(


Can we revert this change, and only keep necessary changes related to StableKey?

georgeh0 · 2026-02-11T19:48:12Z

rust/core/src/engine/execution.rs

                            };
-                            let effect_key = Prof::TargetStateKey::from_bytes(item.key.as_ref())?;
+                            let effect_key: StableKey = storekey::decode(item.key.as_ref())
+                                .map_err(|e| internal_error!("Failed to decode StableKey: {e}"))?;


Let's avoid this map_err() here. It'll strip structured information (like backtrace) from the original error. A simple ? will just work.

georgeh0 · 2026-02-11T19:48:26Z

rust/core/src/engine/execution.rs

            for (target_state_path, target_state) in declared_target_states_to_process {
-                let effect_key_bytes = target_state.key.to_bytes()?;
+                let effect_key_bytes = storekey::encode_vec(&target_state.item_key)
+                    .map_err(|e| internal_error!("Failed to encode StableKey: {e}"))?;


Same here, map_err is not needed.

georgeh0 · 2026-02-12T17:03:02Z

Hi @Geoff-Robin, thanks for contributing! I think the PR is almost there.

Since this is a breaking change, we want to make this PR merged sooner. Let me know if you have any pending changes and want to commit soon. Otherwise, I can also merge the PR first and do a quick cleanup on my side for these comments. Thanks again!

Geoff-Robin · 2026-02-12T17:22:27Z

@georgeh0 Yeah, I've just got started. I'll push them in around an hour.

…e_StableKey

Geoff-Robin · 2026-02-12T18:59:54Z

@georgeh0 I have made the changes completely however
this review couldn't be adressed because because its trait-object error type doesn't satisfy the Sized bound required for automatic conversion to the internal error type.

I'm not sure why the tests are failing on merging with v1 and before. Any idea why it's causing? To my knowledge python tests are failing due to circular imports

georgeh0 · 2026-02-12T19:08:33Z

@georgeh0 I have made the changes completely however this review couldn't be adressed because because its trait-object error type doesn't satisfy the Sized bound required for automatic conversion to the internal error type.

I'm not sure why the tests are failing on merging with v1 and before. Any idea why it's causing? To my knowledge python tests are failing due to circular imports

The actual problem I can see from the failed test is an enum variant (StableKey::Symbol, which just added yesterday) is not handled in your new function:

Geoff-Robin · 2026-02-12T19:10:19Z

Yeah just realised I'll commit it right now

georgeh0 · 2026-02-12T19:33:24Z

Thanks for the contribution! Merging.

Geoff-Robin added 3 commits February 3, 2026 11:11

Using StableKey Instead of PyKey

acc60e9

fixing tests

34c8532

Merge branch 'v1' into feat/Require_target_state_keys_to_fit_into_typ…

6c7f3b2

…e_StableKey

badmonster0 requested a review from georgeh0 February 9, 2026 02:42

georgeh0 reviewed Feb 10, 2026

View reviewed changes

Geoff-Robin added 4 commits February 10, 2026 18:12

Implemented IntoPyObject for PyStableKey

3d3fcaf

Merge branch 'v1' into feat/Require_target_state_keys_to_fit_into_typ…

bb0df25

…e_StableKey

Adressing code comments

d6a01d1

Removing _py argument in declare_target_state_with_child

21a6134

georgeh0 reviewed Feb 11, 2026

View reviewed changes

Geoff-Robin added 2 commits February 12, 2026 00:17

Reverting back to using fingerprint

94eb761

Final touches

02e9f65

georgeh0 reviewed Feb 11, 2026

View reviewed changes

Geoff-Robin added 3 commits February 12, 2026 23:35

Reducing unnecessary changes

33b1ce2

adressing a few more reviews

d055a0e

Merge branch 'v1' into feat/Require_target_state_keys_to_fit_into_typ…

3a26ce4

…e_StableKey

Adding Symbol support in IntoPyObject

bf4c82f

georgeh0 merged commit 90b1f52 into cocoindex-io:v1 Feb 12, 2026
14 checks passed


		use cocoindex_core::state::stable_path::{StableKey, StablePath};

		impl PyStableKey {

Conversation

Geoff-Robin commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Key Changes

Uh oh!

badmonster0 commented Feb 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Geoff-Robin Feb 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgeh0 commented Feb 12, 2026

Uh oh!

Geoff-Robin commented Feb 12, 2026

Uh oh!

Geoff-Robin commented Feb 12, 2026

Uh oh!

georgeh0 commented Feb 12, 2026

Uh oh!

Geoff-Robin commented Feb 12, 2026

Uh oh!

georgeh0 commented Feb 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Geoff-Robin commented Feb 5, 2026 •

edited

Loading

Geoff-Robin Feb 11, 2026 •

edited

Loading