Skip to content

perf(controllers): serve item link search from a name-prefix fast path (backport #59086) - #59092

Open
mihir-kandoi wants to merge 3 commits into
frappe:version-15-hotfixfrom
mihir-kandoi:backport-59086-v15
Open

mihir-kandoi wants to merge 3 commits into
frappe:version-15-hotfixfrom
mihir-kandoi:backport-59086-v15

Conversation

@mihir-kandoi

@mihir-kandoi mihir-kandoi commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

Backport of #59086. Not a cherry-pick: item_query is query builder on develop and raw SQL here, so the same two changes are re-applied to the SQL.

item_query has a leading wildcard on every search column, so no index can serve it and the database scans the whole table. On a 2.4M row catalogue that is ~6s per keystroke. Reported again in #33246.

Stop scanning item_code. Item.autoname ends with self.name = self.item_code on both naming modes, and after_rename writes item_code back to the new name. BaseDocument._sync_autoname_field also resets the column to name on every save, so the two always hold the same value.

Serve a full page of prefix matches from the index. The first ORDER BY key is locate(txt, name), and LOCATE returns 1 when the match starts at the first character, the smallest value that key can take. Every row whose name starts with txt is therefore already in the top-ranked band, so when name LIKE 'txt%' yields a full page, that page is the page the substring search would have produced. Anything short of a full page falls through to the unchanged query. Skipped when txt holds %, _ or \, where the prefix match and the ranking disagree about which rows qualify.

Both paths run the same statement with only the search clause swapped, so the filter conditions, the match conditions, the ranking and the paging cannot drift apart between them. The fallback SQL is byte-identical to what it was, which I verified by reconstructing both strings and comparing them.

TestQueries moves from unittest.TestCase to FrappeTestCase so the framework rolls the new test records back, as it already does for the rest of the suite.


Measured on MariaDB 11.8.9, 3,000,010 Items (2.18 GB table, mixed code shapes), page_len=10, median of 5:

search before after
prefix matching 40k rows 4.6–5.4 s 28 ms
prefix matching 1.2M rows 4.8 s 2.0 s
prefix matching fewer rows than the page 4.6 s 4.5 s
substring only, no prefix match 4.5 s 4.3 s
empty text (fires on field focus) 4.3 s 4.5 s

One query per keystroke, typing a whole code: ITM-0012345 51.4 s → 17.6 s, 4927615 32.1 s → 21.3 s.

The gain is confined to searches whose name-prefix match count is at least the page size. Below that the shortcut cannot prove it has the right page and the unchanged query runs, so a search with no prefix match — a digit block in the middle of a supplier code, say — is unchanged. Dropping the duplicate item_code predicate is correct but not measurable on its own: the scan is I/O bound, not predicate bound.

Numbers come from the raw-SQL implementation in this backport; develop (#59086) emits the same plan shape.

Two # nosemgrep directives are needed on this branch. frappe-sql-format-injection matches .format() and f-strings alike, so the pre-existing finding stops being baselined the moment these lines change, and no form of the statement satisfies it and ruff's UP032 at once. missing-argument-type-hint matches across the whole function, so any edit inside item_query re-fingerprints a pre-existing finding as new — annotating the signature instead would switch on pydantic coercion for a whitelisted method on a stable branch, which is a behaviour change this PR should not carry.

@mihir-kandoi
mihir-kandoi force-pushed the backport-59086-v15 branch 3 times, most recently from bfac2fb to 79f4adb Compare September 16, 2026 06:27
@greptile-apps

greptile-apps Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

The PR appears safe to merge; no actionable new issue was introduced since the previous review.

Reviews (2) · Last reviewed commit: "test(controllers): cover the item_query ..."

…port frappe#59086)

Item.autoname ends with `self.name = self.item_code` on both naming modes,
and after_rename writes item_code back to the new name, so the two columns
always hold the same value. Searching both means a second full-column LIKE
per row for no extra match.

Drop the item_code predicate and keep name, which is the primary key and is
already what the first ORDER BY key ranks on. Output is unchanged on both
engines.

The barcode subquery still joins on item_code; that is a different column
reference and is left alone.
…h (backport frappe#59086)

The substring search has a leading wildcard on every column, so no BTREE
index can serve it and MariaDB scans the whole table. On a 2.4M row catalogue
that is ~6s per keystroke, which makes every transaction form with an Item
field unusable.

The first ORDER BY key is `locate(txt, name)`, and LOCATE returns 1 when the
match starts at the first character, which is the smallest value that key can
take. Every row whose name starts with txt therefore already sits in the
top-ranked band of the existing result.

So when `name LIKE 'txt%'` yields a full page, that page is exactly the page
the substring search would have produced, and it comes from an index range
scan on the primary key instead of a full scan. Anything short of a full page
is inconclusive and falls through to the unchanged query.

Both paths run the same statement with only the search clause swapped, so the
filter conditions, the match conditions, the ranking and the paging cannot
drift apart between them. The fallback SQL is unchanged.

Skipped when txt holds a LIKE wildcard: `_` and `%` make the prefix match and
the LOCATE ranking disagree about which rows qualify, so the shortcut is only
taken for literal text.
frappe#59086)

Each test kills a specific way the shortcut can go wrong: serving a page
without the caller's filters, returning a short prefix page instead of
falling through to the substring search, and taking the shortcut on text
holding a LIKE wildcard, where the prefix match and the LOCATE ranking
disagree about which rows qualify.
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