Support ranges with negative endpoints in search parsing - #4
Conversation
Searches like nilpotency=-1..3 or cm=-4--3 failed because LIST_RE
rejected any range endpoint with a minus sign, and the range splitters
located the separator dash with arg.index('-', 1), which picks the sign
dash in inputs like --4 (produced by prep_ranges from ..-4).
Extend LIST_RE (and LIST_FLOAT_RE) to allow signed endpoints and add
RANGE_DASH_RE, which identifies the separator as the first dash preceded
by a digit or decimal point (so a leading dash stays a minus sign and a
dash after e stays an exponent sign), or a leading dash directly before
a minus sign (omitted lower endpoint). Use it in parse_range,
parse_range2, parse_range3, parse_range_float and parse_ints_to_list;
parse_ints_to_list now also checks .. before -. In parse_range_float,
add the missing open-left branch and fix find_prec's inverted exponent
sign, which made scientific-notation input crash show_float once it
reached the singleton path.
Verified with new unit tests in lmfdb/utils/test_search_parsing.py plus
the galois_groups, maass_forms and number_fields test suites, and by
checking the issue's GaloisGroup nilpotency URLs and EllipticCurve CM
searches through the test client.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The T07 PR added lmfdb/utils/test_search_parsing.py but left the CI test-file census asserting 43 files, so every matrix job failed at the "checking that we didn't miss any test files" step before running any tests or linting. - python-package.yml: bump census count 43 -> 44. - matrix_includes.json: assign the new module to the shard that already runs lmfdb/tests/test_utils.py (both proddb and devmirror copies) so its tests actually execute; add "utils" to that shard's display folders. Coordination: open PR #24 (ai/t28-ui-knowls) adds another new test file needing the same fix. Each PR bumps the census by one for its own file, so whichever of #4/#24 merges second must bump again (44 -> 45): a trivial, intentional same-line conflict on the census line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the external review's P1 finding (CI test-file census): commit 21245d2 bumps the census 43→44 in python-package.yml and registers Coordination note: PR #24 adds another test file with the same census fix — whichever merges second must bump 44→45 (deliberate same-line conflict). Drive-by observation, out of scope: pre-existing |
The PR added lmfdb/knowledge/test_knowledge.py but left the Tests workflow asserting exactly 43 test files, so every matrix job failed at the census step before tests ran. Bump the count to 44 and assign the new module to the knowls shard (alongside test_dynamic_knowls.py) in both the proddb and devmirror matrix entries so it is actually run. Verified: replicated census command (find ... | wc -l == 44) passes; matrix_includes.json parses and lists the file in both shards; sage -python -m pytest lmfdb/knowledge/test_knowledge.py -> 4 passed; pyflakes clean. Note: open PR #4 (ai/t07-negative-ranges) applies the same census bump for its own new test file; the second to merge re-bumps to 45 (trivial same-line conflict, intentional). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
LGTM |
…ranges # Conflicts: # .github/workflows/matrix_includes.json
… (review fix)
Two P1 findings from the second review of this PR.
1. The signed alternatives added to LIST_RE and LIST_FLOAT_RE were ambiguous:
every negative singleton matched both the singleton branch and the branch
with an omitted lower endpoint, so rejecting a comma separated list of n
negative singletons took time exponential in n ("-1," * 25 + "x" took over
two seconds). Both patterns are applied to raw search input in the request
thread, so that is a denial of service risk. Rewrite them with a factored
item grammar in which each comma separated item has exactly one parse;
rejecting 5000 items now takes under a millisecond. A bare "-" is no longer
accepted as an item (it used to be accepted here and fail later in int()).
2. prep_ranges folded every ".." into "-" before the parsers ran, so "..10"
reached parse_ints as "-10" and silently became a search for the value -10
rather than for values at most 10 (and "..4.5" a search near -4.5 in
parse_floats). Fold ".." only when a lower endpoint precedes it, and let
LIST_RE and LIST_FLOAT_RE accept a leading ".."; parse_range2 and
parse_range_float already handled it. Parsers whose own regexes do not
accept ".." (parse_posints, parse_signed_ints, parse_element_of) now reject
"..10" with the usual invalid input message instead of silently searching
for -10.
Verified: sage -python -m pytest on lmfdb/utils/test_search_parsing.py plus the
galois_groups, ecnf, maass_forms and number_fields suites, 79 passed; pyflakes
and ruff clean; test client checks of the issue's Galois group searches, the
ECNF CM discriminant search and the Maass spectral parameter search, inspecting
the query actually sent to postgres (nilpotency=..3 now gives {'$lte': 3}).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…t07-negative-ranges
main gained lmfdb/tests/test_connection_reset.py, which also set the census to 44; this branch had set it to 44 for lmfdb/utils/test_search_parsing.py, so the merge kept 44 without a conflict even though the tree now holds 45 test files. Both new files are already assigned to shards in matrix_includes.json. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
prep_ranges no longer folds a leading .. into a dash, so --3 now reaches LIST_RE only when a user types it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Second review pass requested two P1 changes; both are addressed, and the branch is now merged with 1. Exponential backtracking in
|
| input | query |
|---|---|
/GaloisGroup/?nilpotency=-1..3 |
{'nilpotency': {'$gte': -1, '$lte': 3}} |
/GaloisGroup/?nilpotency=..-1 |
{'nilpotency': {'$lte': -1}} |
/GaloisGroup/?nilpotency=..3 |
{'nilpotency': {'$lte': 3}} (was a search for −3) |
/GaloisGroup/?nilpotency=-1,..3 |
{'$or': [{'nilpotency': -1}, {'nilpotency': {'$lte': 3}}]} |
/EllipticCurve/?cm_disc=-4..-3 |
{'cm': {'$in': [-4, -3]}} |
/ModularForm/GL2/Q/Maass/?spectral_parameter=..9.6 |
{'spectral_parameter': {'$lte': 9.6...}} |
Controls n=2..8, conductor=11-30, discriminant=-3-5 unchanged; - and 3-- still flash errors.
Differential check over ~11,400 random and exhaustive strings: against the previous head, LIST_RE gains only ..-forms and loses only items containing a bare -; LIST_FLOAT_RE gains only ..-forms and loses nothing.
…ew fix)
The rewritten float item grammar still had two parses for one class of item:
a float may end in a decimal point and another may begin with one, so with ..
allowed between two endpoints, 1...2 parses both as 1. .. 2 and as 1 .. .2.
Rejecting a comma separated list of n such items was again exponential in n
("1...2," * 22 + "x" took 1.8 s). No web request path reaches LIST_FLOAT_RE
with that token, since prep_ranges rewrites 1...2 to 1-.2 first, but the
pattern is module level and the claim that every item has one parse was wrong.
prep_ranges already normalizes an internal .. to a dash before either pattern
runs, so .. does not need to be a separator between two endpoints at all; only
the leading .. of an omitted lower endpoint reaches them. Drop the alternative
from both item grammars, keeping the dedicated leading .. branch, and cover
1...2 (plus 1.-.2) in the timeout test.
A sweep over every string of length at most 6 that either pattern accepts as an
item now reports no item whose repetition takes more than 36 microseconds to
reject at 26 copies; the same sweep flags 0...0, 0...1, 1...0 and 1...1 at 29
seconds each on the previous commit. The other list patterns in this file
(LIST_POSINT_RE, LIST_RAT_RE, QQ_LIST_RE, SIGNED_LIST_RE, MULTISET_RE) are
clean under the same sweep.
1..5 and ..5 keep working through parse_ints and parse_floats; a new test
pins that, since the patterns themselves now reject 1..5.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Confirmed, and fixed in The fixTook the first suggested option. INT_ITEM_STR = r"(?:-?\d+(?:-(?:-?\d+)?)?|--\d+|\.\.-?\d+)"
FLOAT_ITEM_STR = r"(?:FLOAT(?:-(?:FLOAT)?)?|--UNSIGNED_FLOAT|\.\.FLOAT)"I applied it to The raw patterns therefore now reject assert not LIST_RE.match("1..5")
parse_ints({"c": "1..5"}, query, "c") # {"c": {"$gte": 1, "$lte": 5}}
parse_ints({"c": "..5"}, query, "c") # {"c": {"$lte": 5}}
parse_floats({"c": "1..5"}, ...) # $gte ≈ 1, $lte ≈ 5
parse_floats({"c": "..5"}, ...) # $lte onlyBetter method, since hand-picked units were the actual failureI replaced the ad-hoc timing spot-check with an exhaustive sweep: enumerate every string up to length 6 over
Verified
CI for |
|
GPT signed off. |
|
Superseded by LMFDB#7141, opened upstream from this same branch. Closing here; review continues upstream. |
Search inputs like
nilpotency=-1..3,nilpotency=-1-3orcm=-4..-1used to flash "not a valid input" becauseLIST_RErejected signed range endpoints, and the splitters located the range dash witharg.index("-", 1), which picks the wrong dash in inputs like--4(whatprep_rangesproduces from..-4). This PR extendsLIST_RE/LIST_FLOAT_REto allow signed endpoints and introducesRANGE_DASH_REto identify the separator dash (first dash preceded by a digit or decimal point, or a leading dash directly before a minus sign), using it consistently inparse_range,parse_range2,parse_range3,parse_range_floatandparse_ints_to_list. A single leading-still denotes a negative number (-5means −5), matching existingparse_range2semantics. Along the way this fixes open-left ranges with negative endpoints (..-4),-4..-1inparse_ints_to_list(used e.g. by ECNF CM-discriminant search), and scientific-notation floats (1e-5,1e-5-2e-4), which previously errored due to an inverted exponent sign infind_prec. Addslmfdb/utils/test_search_parsing.pywith focused unit tests; galois_groups, maass_forms and number_fields suites pass, and the issue's URLs were verified through the test client. Addresses LMFDB#3825.🤖 Generated with Claude Code