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

[search] simplify tsvector generated expression and remove deletedAt condition #7561

Merged
merged 1 commit into from
Oct 10, 2024

Conversation

Weiko
Copy link
Member

@Weiko Weiko commented Oct 10, 2024

Context

Because we can't create GIN indexes on primitive types and in this case both tsvector and deletedAt (date), we had to create an index on the tsvector only and had to make sure it wouldn't return results for deleted records.
To handle this, we added a CASE in the generated expression to generate an empty value if the row has been soft-deleted. This is a bit too complex and won't allow search on softDeleted records in the future.

What we want to do is to make sure deleted records are not returned by default by adding a WHERE clause in the search and still keep good performances by also adding a BTREE index on deletedAt column.

When this was implemented, soft-deleted was not handled properly but now typeorm query builder exclude soft deleted records by default which means the WHERE clause is not necessary. As for the BTREE index on deletedAt, this should be part on a broader effort to add it to all tables and can be added in a later PR.

This PR simply updates the complex tsVectorExpression to only return the expression regardless of the deletedAt column. Search won't return soft-deleted records as explained above and perfs will be improved in an upcoming version.

Test

tested locally with reset db + created a new workspace. Since the feature is not launched yet no more effort should be made.
Screenshot 2024-10-10 at 15 16 16

Note

Some issues with optimistic rendering, we need to refresh once a record has been deleted otherwise it will still be returned by the search

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR simplifies the tsvector generated expression by removing the CASE statement related to the deletedAt column, improving search functionality and performance.

  • Removed CASE statement checking for soft-deleted records in packages/twenty-server/src/engine/workspace-manager/workspace-sync-metadata/utils/get-ts-vector-column-expression.util.ts
  • Updated packages/twenty-server/src/engine/workspace-manager/workspace-sync-metadata/utils/__tests__/get-ts-vectors-column-expression.utils.spec.ts to reflect changes in tsvector expression
  • Relies on TypeORM's default behavior to exclude soft-deleted records instead of manual filtering
  • Improves search performance by simplifying the tsvector expression
  • Paves the way for future addition of BTREE index on deletedAt column for all tables

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@Weiko Weiko merged commit f4bc0c6 into main Oct 10, 2024
8 checks passed
@Weiko Weiko deleted the c--simplify-tsvector-generated-expression branch October 10, 2024 13:30
harshit078 pushed a commit to harshit078/twenty that referenced this pull request Oct 14, 2024
…condition (twentyhq#7561)

## Context
Because we can't create GIN indexes on primitive types and in this case
both tsvector and deletedAt (date), we had to create an index on the
tsvector only and had to make sure it wouldn't return results for
deleted records.
To handle this, we added a CASE in the generated expression to generate
an empty value if the row has been soft-deleted. This is a bit too
complex and won't allow search on softDeleted records in the future.

What we want to do is to make sure deleted records are not returned by
default by adding a WHERE clause in the search and still keep good
performances by also adding a BTREE index on deletedAt column.

When this was implemented, soft-deleted was not handled properly but now
typeorm query builder exclude soft deleted records by default which
means the WHERE clause is not necessary. As for the BTREE index on
deletedAt, this should be part on a broader effort to add it to all
tables and can be added in a later PR.

This PR simply updates the complex tsVectorExpression to only return the
expression regardless of the deletedAt column. Search won't return
soft-deleted records as explained above and perfs will be improved in an
upcoming version.

## Test
tested locally with reset db + created a new workspace. Since the
feature is not launched yet no more effort should be made.
<img width="1289" alt="Screenshot 2024-10-10 at 15 16 16"
src="https://github.com/user-attachments/assets/d9a6f06b-d738-4f1d-8053-2fa93bd8b4f8">

## Note
Some issues with optimistic rendering, we need to refresh once a record
has been deleted otherwise it will still be returned by the search
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants