Probe with longest OR-clause, not shortest

The filter chunker (`filters.chunked`) splits a filter into chunks each
≤ the per-sub-request budget, but bails (returns the full filter
unchanged) when ANY single OR-clause exceeds the budget. So the smallest
filter size the inner chunker can guarantee to emit per sub-request is
bounded below by the LARGEST single clause, not the smallest.

The original implementation used `min(parts)` to model the chunker's
output floor. For filters with uniform clause sizes (all my prior
tests), min == max and the bug was hidden. For filters with lopsided
clauses — e.g. `id='1' OR id='abcdef…long-string'` — using `min` would
let the planner falsely declare a plan feasible. The inner chunker
would then bail on the large clause, the real per-sub-request URL
would carry the full filter, and the request would 414 server-side.

Switch to `max(parts, key=len)`. If singleton+max-clause fits the URL
limit, the inner chunker's budget is ≥ max(parts), so all clauses fit
individually and chunking succeeds. If singleton+max-clause doesn't
fit, the planner correctly raises `RequestTooLarge` instead of
producing an unservable plan.

Regression test: `test_plan_chunks_probes_with_max_clause_not_min`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: thodson-usgs

dataretrieval/waterdata/chunking.py

@@ -19,13 +19,14 @@
   determines output schema and chunking it would shard columns.
 
 Coordination with ``filters.chunked``:
-The planner probes URL length using the SHORTEST top-level OR-clause
+The planner probes URL length using the LONGEST top-level OR-clause
 when a chunkable filter is present, not the full filter. ``filters.
-chunked`` (inner) will split the filter per sub-request, so probing
-with the smallest clause models the per-sub-request URL the stack will
-actually produce. Without this, a long OR-filter plus multi-value
-lists would trigger a premature ``RequestTooLarge`` even though the
-combined chunkers would have made things fit.
+chunked`` (inner) will split the filter per sub-request but bails if
+any single clause exceeds its budget, so the longest clause is the
+smallest filter size the stack is guaranteed to emit. Without this
+coordination, a long OR-filter plus multi-value lists would trigger a
+premature ``RequestTooLarge`` even though the combined chunkers would
+have made things fit.
 """
 
 from __future__ import annotations
@@ -155,23 +156,27 @@ def _chunkable_params(args: dict[str, Any]) -> dict[str, list]:
 
 
 def _filter_aware_probe_args(args: dict[str, Any]) -> dict[str, Any]:
-    """Substitute the filter with its shortest top-level OR-clause if the
+    """Substitute the filter with its LONGEST top-level OR-clause if the
     filter is chunkable, otherwise return ``args`` unchanged.
 
-    The inner ``filters.chunked`` decorator will reduce the filter per
-    sub-request to at most one OR-clause (its hard floor — see
-    ``_chunk_cql_or``). Probing with that minimum models the per-sub-
-    request URL the decorator stack will actually emit, so we don't
-    plan around bytes the filter chunker has already promised to remove.
+    The inner ``filters.chunked`` decorator splits a filter into chunks
+    each ≤ the per-sub-request byte budget, but bails (returns the full
+    filter unchanged) when ANY single OR-clause exceeds the budget. So
+    the smallest filter the inner chunker is guaranteed to emit per
+    sub-request is bounded below by the largest single clause — not the
+    smallest. Probing with ``max(parts, key=len)`` models the worst
+    achievable per-sub-request URL the decorator stack can produce; if
+    that fits, we know the inner chunker won't bail and the actual URL
+    will fit too.
     """
     filter_expr = args.get("filter")
     filter_lang = args.get("filter_lang")
     if not _is_chunkable(filter_expr, filter_lang):
         return args
     parts = _split_top_level_or(filter_expr)
     if len(parts) < 2:
-        return args  # one-clause filter — filter chunker can't shrink it
-    return {**args, "filter": min(parts, key=len)}
+        return args  # one-clause filter — inner chunker can't shrink it
+    return {**args, "filter": max(parts, key=len)}
 
 
 def _worst_case_args(

tests/waterdata_test.py

@@ -225,11 +225,14 @@ def test_filter_aware_probe_args_passes_through_when_not_chunkable():
     assert _filter_aware_probe_args(args) == args
 
 
-def test_filter_aware_probe_args_substitutes_shortest_or_clause():
-    """Chunkable filter → return args with filter replaced by shortest clause."""
+def test_filter_aware_probe_args_substitutes_longest_or_clause():
+    """Chunkable filter → return args with filter replaced by the LONGEST
+    clause. The inner filter chunker bails if any single clause exceeds
+    the per-sub-request budget, so the longest clause is the smallest
+    filter size the chunker can guarantee to emit per sub-request."""
     args = {"filter": "a='1' OR a='22' OR a='333'", "x": 7}
     probe = _filter_aware_probe_args(args)
-    assert probe["filter"] == "a='1'"
+    assert probe["filter"] == "a='333'"
     assert probe["x"] == 7
     assert args["filter"] == "a='1' OR a='22' OR a='333'"  # input not mutated
 
@@ -288,7 +291,9 @@ def test_plan_chunks_coordinates_with_filter_chunker():
         "filter": " OR ".join(clauses),
     }
     # singleton+full-filter ≈ 200 + 10 + 86 = 296 (over limit 240) — would raise.
-    # min-clause probe model ≈ 200 + 10 + 5 = 215 (under limit) — plan succeeds.
+    # max-clause probe model ≈ 200 + 10 + 5 = 215 (under limit) — plan succeeds.
+    # (Here all clauses are the same length, so min==max; the fix matters for
+    # filters with lopsided clauses where min < max.)
     plan = _plan_chunks(args, _fake_build, url_limit=240)
     assert plan is not None  # coordination prevented the premature raise
     assert len(plan["monitoring_location_id"]) > 1  # planner did split
@@ -306,6 +311,25 @@ def test_plan_chunks_coordinates_with_filter_chunker():
         ch._filter_aware_probe_args = saved
 
 
+def test_plan_chunks_probes_with_max_clause_not_min():
+    """Regression: with lopsided OR-clauses (one short, one long), probing
+    with min(parts) lets the planner falsely declare a plan feasible that
+    the inner filter chunker can't actually deliver — it bails when any
+    single clause exceeds the per-sub-request budget. Probing with the
+    longest clause is the safe lower bound on per-sub-request filter
+    size, so the planner correctly raises when no plan can fit."""
+    args = {
+        "sites": ["A" * 10, "B" * 10],
+        "filter": "x='1' OR x='" + "a" * 28 + "'",  # 5-char and 33-char clauses
+    }
+    # base 200 + singleton sites 10 + min-clause 5  = 215  (limit 230 -> fits)
+    # base 200 + singleton sites 10 + max-clause 33 = 243  (limit 230 -> exceeds)
+    # With min: planner succeeds, but real URL with full filter (42) exceeds
+    # 230 -> server 414. With max: planner raises early, as it should.
+    with pytest.raises(RequestTooLarge):
+        _plan_chunks(args, _fake_build, url_limit=230)
+
+
 def test_plan_chunks_still_raises_when_even_min_clause_doesnt_fit():
     """If the limit is so tight that singleton + shortest-clause STILL
     exceeds it, filter chunker can't save us either — raise."""