-
Notifications
You must be signed in to change notification settings - Fork 118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: tx list function name fuzzy search #2102
base: develop
Are you sure you want to change the base?
Conversation
Vercel deployment URL: https://stacks-blockchain-hoasndf9j-hirosystems.vercel.app 🚀 |
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
search_term: Type.Optional( | ||
Type.String({ | ||
description: 'Option to search for transactions by a search term', | ||
examples: ['swap'], | ||
}) | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this also be paired with something like a search_field_name
, which current supports contract_call_function_name
? Seems like there will be use cases were a search will not be interested in every single supported field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is to have the search_term
as a broad search filter that looks into multiple fields. My plan so far is to incrementally add more fields to the same index and test performance, e.g.:
CREATE INDEX idx_search_term_trgm
ON public.txs
USING gin (
contract_call_function_name gin_trgm_ops,
contract_name gin_trgm_ops
memo gin_trgm_ops
....
);
Open to suggestions ofc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems fine to me. In the future we could always add another query param that lets users specify a given field(s) to search. And for now it can default to "all".
c04af8c
to
f663d97
Compare
f663d97
to
7191ba7
Compare
migrations/1727186561690_idx-contract-call-function-name-trgm.js
Outdated
Show resolved
Hide resolved
7191ba7
to
60c3517
Compare
src/datastore/pg-store.ts
Outdated
@@ -1468,6 +1483,15 @@ export class PgStore extends BasePgStore { | |||
const contractIdFilterSql = contractId | |||
? sql`AND contract_call_contract_id = ${contractId}` | |||
: sql``; | |||
|
|||
const pgTrgmInstalled = await this.isPgTrgmInstalled(sql); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be inlined to avoid another round-trip query to postgres? E.g. something like
const searchTermFilterSql = searchTerm
? sql`
AND (
CASE
WHEN EXISTS (
SELECT 1
FROM pg_extension
WHERE extname = 'pg_trgm'
)
THEN similarity(contract_call_function_name, ${searchTerm}) > ${TRGM_SIMILARITY_THRESHOLD}
ELSE contract_call_function_name ILIKE '%' || ${searchTerm} || '%'
END
)
`
: sql``;
60c3517
to
aaebbf7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good to me. My last concern before merging is around query performance. @He1DAr can you work with devops to get this branch deployed to our dev environment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I agree that we should deploy this to dev to give it a test before merging
Implemented fuzzy search for transaction list query using the
pg_trgm
extension, starting with indexing thecontract_call_function_name
column. Future plan is to expand the index to cover more columns, improving search flexibility, e.g.: