fix(python-sdk): harden OQL parser for incomplete filters, negatives, and unclosed quotes - #7510
Conversation
… quotes - Raise ValueError instead of IndexError on incomplete filter strings - Parse negative numeric literals correctly - Reject unterminated double-quoted values - Add unit tests covering the three cases from comet-ml#7508 Fixes comet-ml#7508
github-actions
Bot
added
python
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| ): | ||
| self._cursor += 1 |
There was a problem hiding this comment.
Duplicate quote scanner in parser
The new quoted-value path duplicates the cursor-walk and closing-quote checks already in _check_escaped_key() + _is_valid_escaped_key_char(), so quote-handling fixes now have to stay in sync in two places and it's easy to drift on boundary checks or error wording — should we factor a shared quoted-token helper and reuse it here?
Want Baz to fix this for you? Activate Fixer
|
Addressed the review note: quoted values now go through a shared |
| if parsed_field not in supported_operators: | ||
| parsed_field = "default" | ||
|
|
||
| if self._cursor >= len(self.query_string): | ||
| raise ValueError("Incomplete filter string: unexpected end of input") | ||
|
|
There was a problem hiding this comment.
Incomplete filter end-of-input error
_parse_operator now raises ValueError("Incomplete filter string: unexpected end of input") at end-of-input, but sdks/python/tests/unit/api_objects/test_opik_query_language.py only covers for_traces(), so the sibling public APIs that share this parser still lack malformed-input coverage — should we add matching cases there?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/api_objects/opik_query_language.py` around lines 624-635,
`_parse_operator` raises `ValueError("Incomplete filter string: unexpected end of
input")` when `self._cursor >= len(self.query_string)`. Update
`sdks/python/tests/unit/api_objects/test_opik_query_language.py` so that malformed-input
coverage for the sibling public factory APIs (e.g., `for_spans`, `for_threads`,
`for_dataset_items`, `for_prompt_versions`), not just `for_traces`, asserts the same
exception and message for a query that ends after a valid field (no operator). Refactor
the test into a parametrized/looped structure over the constructors to avoid duplicating
cases and to keep the coverage aligned across all configs.
Extend malformed-input coverage beyond for_traces so for_spans, for_threads, for_dataset_items, and for_prompt_versions also assert Incomplete filter string and missing-close quote errors (Baz review). Also align the for_traces unterminated-value expectation with the shared quote-scanner message.
|
Addressed the latest Baz review note:
Happy to take any further maintainer feedback. |
…ase tests _parse_field's quoted-key loop let an alnum char at end-of-string satisfy the field-char branch before ever checking quote closure, so an unterminated key ending in an alnum char silently overflowed the cursor instead of raising "Missing closing quote". Also fixes ruff-format failure and adds test coverage for escaped quotes in values, decimal/negative-sign edge cases, and whitespace-only queries.
petrotiurin
left a comment
petrotiurin
left a comment
There was a problem hiding this comment.
Looks good to me, thanks for enhancing the parser! I've added some additional tests to ensure the edge-case coverage.
4 participants
Summary
Fixes three OQL parser issues reported in #7508:
duration >,name =, trailingand) raised bare IndexError - now raise ValueError with a clear incomplete-filter message.duration > -5) failed with trailing-characters errors - leading minus is now consumed correctly.name = "hello) were silently accepted - now raise missing-closing-quote ValueError (aligned with quoted keys).Test plan
Closes #7508