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: highlighted views and queries #1953

Merged
merged 10 commits into from
May 30, 2024
Merged

feat: highlighted views and queries #1953

merged 10 commits into from
May 30, 2024

Conversation

capJavert
Copy link
Contributor

@capJavert capJavert commented May 29, 2024

Implementation from

  • see which columns need indexes

@capJavert capJavert self-assigned this May 29, 2024
@capJavert capJavert changed the title feat: trending and popular materialized views and queries feat: highlighted views and queries May 29, 2024
Copy link
Contributor

@rebelchris rebelchris left a comment

Choose a reason for hiding this comment

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

I'm happy with it.
We can do the indexes based on feedback from everSQL even, since they not used heavily in the beginning it should be fine :)

@rebelchris rebelchris marked this pull request as ready for review May 30, 2024 00:17
@rebelchris rebelchris requested a review from a team as a code owner May 30, 2024 00:17
Copy link
Member

@sshanzel sshanzel left a comment

Choose a reason for hiding this comment

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

I think it would be a good idea to always include a test for registering the worker, moving forward. Looks good to me 🚀

import cron from '../../src/cron/updateHighlightedViews';

describe('updateHighlightedViews cron', () => {
it('should be registered', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I like this. Registering the worker gets missed sometimes and this would ensure they got listed.

Copy link
Contributor

Choose a reason for hiding this comment

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

100% agreed on this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, all the recent crons/worker have this 👌

Copy link
Member

@idoshamun idoshamun left a comment

Choose a reason for hiding this comment

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

I haven't fully compared the queries here to my suggestion so just double check they match

@rebelchris
Copy link
Contributor

@idoshamun // @omBratteng // @nensidosari I've now added all queries/test and cleaned up unused field in MVs
Could at least one of you sanity check before I merge? 🙏

Copy link
Contributor

@nensidosari nensidosari left a comment

Choose a reason for hiding this comment

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

Looks great

@rebelchris rebelchris merged commit c0758a0 into main May 30, 2024
8 checks passed
@rebelchris rebelchris deleted the trending-popular-mv branch May 30, 2024 12:32
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.

6 participants