From b7a3a2760f46dbdff1e0cce307bfb874944244b4 Mon Sep 17 00:00:00 2001 From: Andrew Chen <48723787+chuenchen309@users.noreply.github.com> Date: Sat, 18 Jul 2026 18:47:25 +0800 Subject: [PATCH] =?UTF-8?q?fix(metadata):=20Infinity=20push-down=20must=20?= =?UTF-8?q?reject=20multi-valued-unsafe=20negative=20ops=20(=E2=89=A0,=20n?= =?UTF-8?q?ot=20in)=20(#16943)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### What problem does this PR solve? The Elasticsearch and Infinity metadata push-down translators are meant to be interchangeable pre-filters that both mirror the in-memory `meta_filter` fallback — the shared test section is even titled *"is_pushdown_supported pre-check (same logic for both backends)"*. But the two `is_pushdown_supported` implementations diverge: - `common/metadata_es_filter.py` defines `MULTIVALUE_UNSAFE_NEGATIVE_OPS = frozenset({"≠", "not in"})` and refuses push-down for those operators. - `common/metadata_infinity_filter.py` has **no such guard** and pushes them down. **Why the guard exists:** `meta_fields.` can hold a JSON array, and the in-memory `meta_filter` matches a document when **any** of its values satisfies the predicate (per-value-bucket semantics). A document whose `tag` is `[a, b]` therefore still matches `tag ≠ a` — bucket `b` satisfies it. The Infinity push-down emits `NOT JSON_CONTAINS(meta_fields, '$.tag', '"a"')`, which means *"the array contains no `a` at all"*, so it **silently drops** that document. Same divergence for `not in`. The result: `tag ≠ a` / `tag not in (...)` under-counts results for any document that has the excluded value alongside others, but only on the Infinity backend. This is on a live production path: `DocMetadataService._filter_doc_ids_by_metadata_infinity` (`api/db/services/doc_metadata_service.py:948`) calls this exact `is_pushdown_supported` as the sole gate before building the Infinity SQL, mirroring the ES branch at line 880 which uses the guarded ES version. Reproduction (both real modules, no services needed): ```python >>> from common import metadata_es_filter as es, metadata_infinity_filter as inf >>> f = [{"op": "≠", "key": "tag", "value": "a"}] >>> es.is_pushdown_supported(f), inf.is_pushdown_supported(f) (False, True) # ES falls back to in-memory (correct); Infinity pushes down (wrong) ``` ### Type of change - [x] Bug Fix (non-breaking change which fixes an issue) ### Fix Add the same `MULTIVALUE_UNSAFE_NEGATIVE_OPS` set to `metadata_infinity_filter` and reject those operators in `is_pushdown_supported`, so a single such filter forces the whole request to the in-memory path — the only place the per-bucket semantics are reproduced. `not contains` is intentionally still allowed, matching the ES backend (`all(not contains)` == `not any(contains)`, which the push-down expresses correctly on multi-valued fields). The `≠` / `not in` translators themselves are unchanged — they remain correct for the in-memory-fallback path; only the push-down eligibility gate is fixed. ### Testing - Confirmed `is_pushdown_supported([{op}])` now returns `False` for `≠` and `not in` on **both** backends (previously ES=False, Infinity=True). - Added `test_pushdown_check_rejects_multivalue_unsafe_negative_ops`, which asserts both backends reject these ops. Confirmed red→green: it fails against the pre-fix Infinity module, passes after. - `ruff check` / `ruff format --check` clean. ### Note on overlap Open PR #16833 also edits `test/unit_test/common/test_metadata_filter.py`, but only **appends** at the end of the file (line 640+) and changes `metadata_es_filter.py` / `metadata_utils.py`, not the Infinity module — no overlap with this change. ### Disclosure AI-assisted (Claude Code): the divergence was surfaced by an AI-assisted review pass, but I independently reproduced it against the real modules, confirmed the production call path, and verified the fix and test before submitting. Signed-off-by: Andrew Chen <48723787+chuenchen309@users.noreply.github.com> Signed-off-by: chuenchen309 <48723787+chuenchen309@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 --- common/metadata_infinity_filter.py | 16 ++++++++++++++++ test/unit_test/common/test_metadata_filter.py | 14 ++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/common/metadata_infinity_filter.py b/common/metadata_infinity_filter.py index 238afd3596..c49174b9aa 100644 --- a/common/metadata_infinity_filter.py +++ b/common/metadata_infinity_filter.py @@ -55,6 +55,20 @@ _RANGE_OPS: Dict[str, str] = { "≤": "<=", } +# Negative operators whose legacy per-value-bucket semantics cannot be +# reproduced by a single push-down predicate over a multi-valued metadata +# field. ``meta_fields.`` may hold a JSON array, and the in-memory +# ``meta_filter`` matches a doc when ANY of its values satisfies the predicate; +# e.g. a doc whose ``tag`` is ``[a, b]`` still matches ``tag ≠ a`` (via bucket +# ``b``). ``NOT JSON_CONTAINS(...)`` instead means "the array contains no ``a`` +# at all", which would silently drop that doc. Without a cheap way to prove a +# field is single-valued at query time we refuse push-down for these operators +# and let the in-memory fallback handle them. Mirrors the identically named set +# in ``metadata_es_filter``. ``not contains`` is intentionally excluded: +# ``all(not contains)`` equals ``not any(contains)``, which the push-down +# expresses correctly on both single- and multi-valued fields. +MULTIVALUE_UNSAFE_NEGATIVE_OPS: frozenset[str] = frozenset({"≠", "not in"}) + class MetaFilterTranslator: """Translate one user filter clause at a time into Infinity SQL filter strings.""" @@ -209,6 +223,8 @@ def is_pushdown_supported(filters: Sequence[Dict[str, Any]]) -> bool: op = flt.get("op") if op not in SUPPORTED_OPERATORS: return False + if op in MULTIVALUE_UNSAFE_NEGATIVE_OPS: + return False if not isinstance(flt.get("key"), str) or not flt.get("key"): return False return True diff --git a/test/unit_test/common/test_metadata_filter.py b/test/unit_test/common/test_metadata_filter.py index 62f69c9101..7ea3262d25 100644 --- a/test/unit_test/common/test_metadata_filter.py +++ b/test/unit_test/common/test_metadata_filter.py @@ -61,6 +61,20 @@ def test_pushdown_check_accepts_not_contains(): assert is_pushdown_supported([{"key": "tag", "op": "not contains", "value": "x"}]) +@pytest.mark.parametrize("op", ["≠", "not in"]) +def test_pushdown_check_rejects_multivalue_unsafe_negative_ops(op): + # These negative operators cannot be expressed as a single push-down + # predicate over a multi-valued metadata field without diverging from the + # in-memory meta_filter's per-value-bucket semantics, so both backends must + # refuse push-down and fall back to in-memory evaluation. The ES backend + # already does this; the Infinity backend must match it. + from common.metadata_es_filter import is_pushdown_supported as es_is_pushdown_supported + + flt = [{"key": "tag", "op": op, "value": "a"}] + assert not is_pushdown_supported(flt) + assert not es_is_pushdown_supported(flt) + + # --------------------------------------------------------------------------- # Shared: plan_pushdown (same logic for both backends) # ---------------------------------------------------------------------------