Skip to content

feat: Fetching index suggestions and dealing with the states CLOUDP-311786 #6887

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

Merged
merged 14 commits into from
May 7, 2025

Conversation

rubydong
Copy link
Collaborator

@rubydong rubydong commented May 1, 2025

Description

https://jira.mongodb.org/browse/CLOUDP-311786

With data:
image

Loading:
image

NOTE: Error state to be fully implemented in another ticket, and create index with suggested index will also be in a separate ticket

Checklist

  • New tests and/or benchmarks are included
  • Documentation is changed or added
  • If this change updates the UI, screenshots/videos are added and a design review is requested
  • I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@github-actions github-actions bot added the feat label May 1, 2025
@rubydong rubydong added the no release notes Fix or feature not for release notes label May 1, 2025
@rubydong rubydong marked this pull request as ready for review May 5, 2025 15:40
Copy link
Member

@Anemy Anemy left a comment

Choose a reason for hiding this comment

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

Looking good, left a couple questions/thoughts!

);

const query = mql.parseQuery(
EJSON.parse(inputQuery, { relaxed: false }),
Copy link
Member

Choose a reason for hiding this comment

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

Does this work as expected with some queries? On a quick glance we might need to be parsing them with a different method. Should it be using this instead?

const parsed = _parseShellBSON(source, { mode: ParseMode.Loose });

I might be wrong, haven't checked out the branch and tried it yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it works for the most part but will break if there's a new line

working:
image

breaks:
image

Copy link
Member

Choose a reason for hiding this comment

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

Does using an ObjectId type work?

{ _id: ObjectId("578f6fa2df35c7fbdbaed8f2") }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it did not so i updated it. now it works with object
image

Copy link
Member

@Anemy Anemy left a comment

Choose a reason for hiding this comment

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

lgtm!

if (sampleDocuments === null) {
try {
sampleDocuments =
(await dataService.sample(namespace, { size: 50 })) || [];
Copy link
Member

Choose a reason for hiding this comment

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

I should mention, ideally we'd be passing an abort signal here, that way we can cancel the operation if the user closes the tab or something. I think this is fine as is for now, but something to keep in mind for future asynchronous things that allow for abort signals.

@rubydong rubydong merged commit 2e115df into main May 7, 2025
79 of 82 checks passed
@rubydong rubydong deleted the cloudp-311786 branch May 7, 2025 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat no release notes Fix or feature not for release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants