From 198815a3650be98e4092d6b40c28832a3947807f Mon Sep 17 00:00:00 2001 From: Andrew Chen <48723787+chuenchen309@users.noreply.github.com> Date: Thu, 16 Jul 2026 13:42:08 +0800 Subject: [PATCH] fix(metadata): meta_filter loses case-insensitive matches and leaks state across dict entries (#16956) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What `meta_filter()`'s in-memory `filter_out()` helper has two related bugs in how it coerces `input`/`value` for comparison operators (`=`, `≠`, `>`, `<`, `≥`, `≤`): **1. Asymmetric commit on partial `literal_eval` failure.** The original code: ```python input = ast.literal_eval(input) value = ast.literal_eval(value) ``` runs as two separate statements inside one `try`. If the first succeeds and the second raises, the first assignment already committed — `input` and `value` end up as different types, and the subsequent `.lower()` case-folding silently no-ops for whichever side didn't get lowered as a string. Concretely: metadata cell `"None"` is a valid Python literal (`ast.literal_eval("None")` → `None`), but a query value `"none"` (lowercase) is not — so `status = "none"` never matches a cell whose value is `"None"`, even though the intended semantics are case-insensitive. **2. `value` mutated in place, reused across dict entries.** `filter_out(v2docs, operator, value)` loops over every `(input, docids)` pair in `v2docs` and coerces `value` inside the loop body without resetting it — so once one entry's `literal_eval(value)` succeeds and rebinds `value` to a non-string, every later entry in the same call compares against that already-coerced leftover instead of the original filter value. ## Fix - Commit both `literal_eval` results together via tuple assignment (`input, value = ast.literal_eval(input), ast.literal_eval(value)`), so a failure on either side leaves both operands in their pre-coercion form instead of a mismatched mix. - Save the original `value` before the loop and reset it at the top of each iteration, so per-entry coercion never leaks into the next entry. ## Testing Added 3 tests to `test/unit_test/common/test_metadata_filter_operators.py` covering both symptoms (case-insensitive match against a metadata cell that's a Python keyword literal, both `=` and `≠`; a numeric `>` comparison unaffected by an earlier dict entry having coerced the query value). Confirmed red on `common/metadata_utils.py` at HEAD (`git stash` the fix, tests fail with the exact symptom described above), green after. Full existing `test_metadata_filter_operators.py` suite (22/22, including the 3 new tests) passes. `ruff check` and `ruff format --check` clean on both touched files. Sandbox note: this environment has no network access to install `pytest`/`pytest-asyncio`, so tests were run by importing the test module and invoking each `test_*` function directly (same approach as prior PRs from this account against this repo, e.g. #16949). `test_apply_semi_auto_meta_data_filter.py` (the other file exercising `meta_filter` indirectly through `apply_meta_data_filter`) needs `pytest-asyncio` + heavier mocking and wasn't run, but it exercises `apply_meta_data_filter`'s async/LLM-filter-generation path, not `filter_out`'s coercion logic touched here. Cross-referenced open PR #16833 (also touches `common/metadata_utils.py`) — confirmed via `gh pr diff` it only touches `convert_conditions`/operator-alias normalization and `apply_meta_data_filter`'s `None`-vs-`["-999"]` sentinel logic, not `filter_out`'s comparison-coercion code path. No overlap. --- This PR was drafted with AI assistance (Claude); I reviewed the change, independently reproduced both symptoms, and take responsibility for it. Signed-off-by: chuenchen309 <48723787+chuenchen309@users.noreply.github.com> --- common/metadata_utils.py | 16 +++++++++-- .../common/test_metadata_filter_operators.py | 28 +++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/common/metadata_utils.py b/common/metadata_utils.py index 130413402d..3aa4c7c0e6 100644 --- a/common/metadata_utils.py +++ b/common/metadata_utils.py @@ -39,7 +39,13 @@ def meta_filter(metas: dict, filters: list[dict], logic: str = "and"): def filter_out(v2docs, operator, value): ids = [] + original_value = value for input, docids in v2docs.items(): + # Reset to the pristine filter value each iteration -- the comparison branch + # below reassigns `value` in place (date normalization, literal_eval coercion), + # and reusing that mutated value on the next dict entry compares it against + # the wrong (already-coerced) type/content instead of the original filter value. + value = original_value if operator in ["=", "≠", ">", "<", "≥", "≤"]: # Check if input is in YYYY-MM-DD date format input_str = str(input).strip() @@ -64,8 +70,14 @@ def meta_filter(metas: dict, filters: list[dict], logic: str = "and"): try: if isinstance(input, list): input = input[0] - input = ast.literal_eval(input) - value = ast.literal_eval(value) + except Exception: + pass + # Commit both literal_eval results together, or neither -- assigning + # just one side (e.g. "None" parses but "none" doesn't) would compare + # mismatched types after lowercasing and silently break the + # case-insensitive match below. + try: + input, value = ast.literal_eval(input), ast.literal_eval(value) except Exception: pass diff --git a/test/unit_test/common/test_metadata_filter_operators.py b/test/unit_test/common/test_metadata_filter_operators.py index fc0cca7be8..dea5e3ef90 100644 --- a/test/unit_test/common/test_metadata_filter_operators.py +++ b/test/unit_test/common/test_metadata_filter_operators.py @@ -164,3 +164,31 @@ def test_or_logic_still_unions_after_empty_first_condition(): ] assert set(meta_filter(metas, filters, logic="or")) == {"doc2"} + + +def test_equal_is_case_insensitive_for_python_keyword_literals(): + # "None" is a metadata cell that happens to be a Python literal keyword; the + # query value "none" (lowercase) is not a valid literal on its own. Coercing + # one side (input -> None) while leaving the other as the string "none" would + # compare mismatched types and silently fail the case-insensitive match. + metas = {"status": {"None": ["doc1"], "Active": ["doc2"]}} + filters = [{"key": "status", "op": "=", "value": "none"}] + + assert meta_filter(metas, filters) == ["doc1"] + + +def test_not_equal_is_case_insensitive_for_python_keyword_literals(): + metas = {"status": {"None": ["doc1"], "Active": ["doc2"]}} + filters = [{"key": "status", "op": "≠", "value": "none"}] + + assert meta_filter(metas, filters) == ["doc2"] + + +def test_greater_than_unaffected_by_prior_dict_entry_coercing_the_query_value(): + # The query value must be re-read fresh for every metadata entry -- if an + # earlier entry coerces it in place (e.g. "5" -> 5), a later entry must not + # compare against that already-coerced leftover instead of the original "5". + metas = {"score": {"5": ["doc1"], "10": ["doc2"]}} + filters = [{"key": "score", "op": ">", "value": "5"}] + + assert meta_filter(metas, filters) == ["doc2"]