added ability to set a single value instead of relying on a list of v…#261
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.
Reviewed commit: e872aebfaf
ℹ️ 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 "@codex address that feedback".
| if isinstance(values, (str, bytes)): | ||
| return [values] | ||
| if isinstance(values, collections.abc.Iterable): | ||
| return values | ||
| return [values] |
There was a problem hiding this comment.
The new single-value support wraps scalars into a one‑element list, but _extract_fields_with_provenance zips all scope iterables and stops at the shortest. That means a call like scope_fields={"student": "bob", "doc_id": [..many..]} will yield only one key instead of applying student across all doc_ids, contradicting the documentation and silently truncating results whenever any other scope axis has more than one value. Consider broadcasting single values to match the longest iterable instead of returning a length‑1 list.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7a530a527
ℹ️ 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 "@codex address that feedback".
| iterables = [_expand_scope_values(values, repeat=True) for _, values, _ in scope_specs] | ||
| async for items in _async_zip_many(iterables): |
There was a problem hiding this comment.
When multiple scope fields are provided as single values, each value is wrapped as _ScopeSingleton and then expanded with repeat=True, which returns the infinite _repeat_value generator for every field. _async_zip_many only stops when any generator is exhausted, so with all-infinite generators it never terminates, causing handle_keys to hang instead of yielding a single key. This happens for inputs like student='s1' and doc_id='d1' in the multi-scope path; consider detecting the “all singletons” case and emitting one tuple.
Useful? React with 👍 / 👎.
| def _normalize_scope_values(values): | ||
| if values is None: | ||
| return _ScopeSingleton(None) | ||
| if isinstance(values, collections.abc.AsyncIterable): | ||
| return values | ||
| if isinstance(values, dict): | ||
| return values |
There was a problem hiding this comment.
_normalize_scope_values returns dicts unchanged, and _expand_scope_values only repeats _ScopeSingleton values. In a multi-scope query, passing a single dict as a value (common when a scope item is a dict) will produce a generator that yields once and then stops, so _async_zip_many will truncate to a single combination instead of repeating that dict across the other scope values. This makes the new “single value applied across all items” behavior inconsistent for dict inputs.
Useful? React with 👍 / 👎.
bradley-erickson
force-pushed
the
berickson/20260130-execution-dag-single-value
branch
from
203e245 to
1310c89
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1310c89e69
ℹ️ 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 "@codex address that feedback".
| # Multiple dimensions: zip with broadcasting for single values | ||
| iterables = [_expand_scope_value(values, broadcast=True) for _, values, _ in scope_specs] | ||
| async for items in _async_zip_many(iterables): |
There was a problem hiding this comment.
When there are multiple scope dimensions, single values are expanded with broadcast=True, which uses _repeat_forever and makes each dimension an infinite async iterator. If every scope field is a single value (e.g., scope_fields={"student": "bob", "doc_id": "doc1"} or STUDENTS="bob" + RESOURCES="doc1"), _async_zip_many never hits StopAsyncIteration, so _extract_fields_with_provenance yields tuples forever and the query will hang. This is a regression introduced by the new SingleValue broadcasting; the multi-dimension case needs a finite termination condition when all dimensions are singletons.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
…alues