Skip to content
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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

He1DAr
Copy link
Collaborator

@He1DAr He1DAr commented Oct 1, 2024

Implemented fuzzy search for transaction list query using the pg_trgm extension, starting with indexing the contract_call_function_name column. Future plan is to expand the index to cover more columns, improving search flexibility, 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
  ....
);

Copy link

github-actions bot commented Oct 1, 2024

Vercel deployment URL: https://stacks-blockchain-hoasndf9j-hirosystems.vercel.app 🚀

Copy link

codecov bot commented Oct 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@He1DAr He1DAr marked this pull request as draft October 1, 2024 22:25
src/datastore/pg-store.ts Outdated Show resolved Hide resolved
Comment on lines +116 to +121
search_term: Type.Optional(
Type.String({
description: 'Option to search for transactions by a search term',
examples: ['swap'],
})
),
Copy link
Member

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.

Copy link
Collaborator Author

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.

Copy link
Member

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".

@He1DAr He1DAr force-pushed the feat/tx-list-function-name-fuzzy-search branch 2 times, most recently from c04af8c to f663d97 Compare October 3, 2024 13:09
@He1DAr He1DAr force-pushed the feat/tx-list-function-name-fuzzy-search branch from f663d97 to 7191ba7 Compare October 3, 2024 13:14
@He1DAr He1DAr marked this pull request as ready for review October 3, 2024 13:14
@He1DAr He1DAr requested review from zone117x and rafaelcr October 3, 2024 13:14
@He1DAr He1DAr force-pushed the feat/tx-list-function-name-fuzzy-search branch from 7191ba7 to 60c3517 Compare October 10, 2024 20:18
@He1DAr He1DAr requested a review from rafaelcr October 10, 2024 20:26
@@ -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);
Copy link
Member

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``;

@He1DAr He1DAr force-pushed the feat/tx-list-function-name-fuzzy-search branch from 60c3517 to aaebbf7 Compare October 11, 2024 14:46
@He1DAr He1DAr requested a review from zone117x October 11, 2024 15:10
Copy link
Member

@zone117x zone117x left a 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?

Copy link
Collaborator

@rafaelcr rafaelcr left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🆕 New
Development

Successfully merging this pull request may close these issues.

3 participants