Skip to content

fix(search): keep a limit the caller wrote into the ask query - #161

Merged
LukasGold merged 2 commits into
mainfrom
fix/semantic-search-respect-query-limit
Sep 3, 2026
Merged

fix(search): keep a limit the caller wrote into the ask query#161
LukasGold merged 2 commits into
mainfrom
fix/semantic-search-respect-query-limit

Conversation

@LukasGold

@LukasGold LukasGold commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Noted as out of scope while probing #150.

The bug

semantic_search appended the default limit unconditionally:

single_query += f"|limit={query.limit}"

SMW honours the last limit in the query string, so [[Category:Item]]|limit=2 went out as [[Category:Item]]|limit=2|limit=1000 and the caller's limit was silently ignored. Verified live against a wiki: it returned 1000 results, not 2.

Changes

  • get_query_limit(query): returns the limit an SMW ask query sets itself, None if it sets none.
  • semantic_search: append SearchParam.limit only to a query that does not set its own. A query that does is sent unchanged.
  • The truncation warning from fix: handle empty SMW ask results in semantic_search #150 now compares against the limit in force rather than SearchParam.limit.
  • limit=0 no longer triggers that warning.
  • SearchParam.limit=None now means "apply no limit and leave it to the wiki", instead of raising TypeError.
  • Precedence and the None meaning documented on SearchParam.limit and in the semantic_search docstring.
  • 11 parametrised cases for get_query_limit, 9 tests for semantic_search, all offline.

Why the truncation warning had to change with it

#150 warns when n >= query.limit. Once a caller's |limit=2 is honoured, results are truncated at 2 while query.limit is still the default 1000, so 2 >= 1000 is false and the warning would never fire on exactly the queries most likely to hit their limit. The effective limit is now a local, used for both the append decision and the warning.

limit=0 follows from the same local: 0 >= 0 would warn "returned 0 results, which meets the requested limit of 0. Results are truncated" on every count-style query. A zero limit asks for no results, so meeting it says nothing about truncation.

SearchParam.limit = None

limit is typed Optional[int], but None was never usable: it appended the literal |limit=None and then raised TypeError on n >= None. src/osw/data/import_utility.py:809-823 works around it by building a SearchParam without the field at all when its own limit argument is None.

None now means what the type suggests: send no limit parameter and leave the wiki to apply its own default. No limit is then in force, so no truncation warning is emitted, since the result count says nothing about completeness.

Nothing can regress: every previous limit=None call raised.

The import_utility workaround is left in place. It is not equivalent to passing None through: omitting the field yields the 1000 default, while None would now yield SMW's own, much smaller default. Collapsing the branch would change how many results that caller gets.

Parsing the limit out of a query

| is not a plain separator in SMW: || is the disjunction operator inside [[...]] conditions, so [[Has p::a||limit=2]] would split into a bogus limit=2]] segment. Conditions are removed before the parameters are split, and the parameter pattern is anchored:

CONDITION_PATTERN = re.compile(r"\[\[.*?\]\]", re.DOTALL)
LIMIT_PARAM_PATTERN = re.compile(r"^limit\s*=\s*(\d+)$", re.IGNORECASE)
  • Non-greedy \[\[.*?\]\] can only remove a prefix of a nested condition, never text past a ]], so it cannot swallow a following |limit=.
  • Anchoring at ^limit keeps the printout |?limit and the printout parameter |?Has subobject|+limit=3 from being read as the query limit. Both limit something other than the query.
  • Multiple limits: the last wins, matching SMW.
  • limit= with a non-numeric value returns None, so the default is appended after it. ...|limit=all|limit=1000 is a well-formed parameter list and SMW takes the last, which is the behaviour before this change.

Behaviour change

  • A query string carrying limit= now wins over SearchParam.limit. Previously the SearchParam value always won, which is the bug.
  • The truncation warning can now name a smaller limit than SearchParam.limit.
  • SearchParam.limit=None sends no limit instead of raising. No caller could have relied on the old behaviour.

Not covered

The truncation warning still only catches truncation at the requested limit. SMW additionally caps every request server-side at $smwgQMaxLimit, so a request above that cap is silently reduced and returns fewer results than asked for without any warning. Tracked in #162, which also sketches reading SMW's own query.meta.hasFurtherResults flag instead of inferring truncation from the result count.

Verification

Offline suite passes (156 passed, 1 skipped). No other call site in the repo builds an SMW ask string containing a limit, so nothing else changes behaviour.

- semantic_search appended |limit= unconditionally, and SMW honours the
  last limit, so '[[Category:Item]]|limit=2' was sent as ...|limit=1000
- get_query_limit() reads the limit a query sets itself, ignoring
  conditions, printouts and printout parameters
- the default is appended only to a query that sets no limit
- the truncation warning now compares against the limit in force
- 'limit=0' no longer warns about truncation
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Release preview

Merging this PR would release v2.2.1 (current: v2.2.0).

Changelog preview (truncated)
## v2.2.1 (2026-09-02)

### Bug Fixes

- **search**: Keep a limit the caller wrote into the ask query
  ([`0c75699`](https://github.com/OpenSemanticLab/osw-python/commit/0c756993bcf9a9114c3072ee03037757aaa5fcf5))

- **search**: Treat a SearchParam limit of None as no limit
  ([`6e94187`](https://github.com/OpenSemanticLab/osw-python/commit/6e94187c455942afdec2746a498c44c6fd5953e0))

Preview via python-semantic-release and conventional commits.

- 'limit=None' appended the literal '|limit=None' and then raised
  TypeError on the truncation check
- None now sends no limit parameter and leaves the wiki to apply its own
- no truncation warning in that case, since no limit is in force
@LukasGold
LukasGold merged commit eb59627 into main Sep 3, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant