Support lazy Python mappings via dynamic MapValue #24
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
| fn get(&self, key: &Key) -> Option<Value> { | ||
| Python::with_gil(|py| { | ||
| let bound = self.mapping.bind(py); | ||
| let mapping = bound.downcast::<PyMapping>().ok()?; | ||
| let py_key = Self::key_to_python(py, key); | ||
| let value = mapping.get_item(py_key).ok()?; | ||
| RustyPyType(&value).try_into_value().ok() | ||
| }) |
There was a problem hiding this comment.
When a value in a lazy Python mapping fails to convert into a CEL Value, PyMappingValue::get returns None and the evaluator treats the access as a missing key (null). This silently discards the underlying Python exception, whereas the previous eager conversion of PyDict raised a ConversionError immediately. A dict subclass that yields an unsupported object for data["foo"] will now evaluate to null instead of raising, making bad data hard to detect. It would be better to surface the conversion failure (e.g., by raising or logging) rather than swallowing it as an absent key.
Useful? React with 👍 / 👎.
Summary
MapValuetrait so maps can be backed by dynamic sourcesPyMappingValueand surface them to CEL without eagerly copying entries__getitem__calls are evaluated correctlyTesting
cargo test(fails: missing libpython3.11.so.1.0 in test harness)pytest(fails: environment lacks installedcelpackage and mktestdocs dependency)https://chatgpt.com/codex/tasks/task_b_68ede34e85148329ad0078eeea94b756