Skip to content

feat: optional field count threshold #231

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 4 commits into from
Feb 19, 2025
Merged

feat: optional field count threshold #231

merged 4 commits into from
Feb 19, 2025

Conversation

paula-stacho
Copy link
Contributor

@paula-stacho paula-stacho commented Feb 18, 2025

Again an optional parameter. I had hard time coming up with the name, let me know if you have better suggestion. This one is a hard limit that can be imposed on the complexity (represented by unique field count). Together with the lower stored samples limit, it can be used as a prevention mechanism for running out of memory during the analysis itself (https://jira.mongodb.org/browse/COMPASS-8526), or later down the line (such as the new schema export conversion or file download).

@paula-stacho paula-stacho marked this pull request as ready for review February 18, 2025 16:22
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.
One thing that does come to mind is that with the new interface and options, the README of this github repo is still only showing the old way of parsing the schema. I don't think we need to do it now, however if we feel more folks are going to try it out we could spend some time to improve that documentation.

@@ -167,6 +167,7 @@ type AllSchemaParseOptions = {
storeValues: boolean;
signal?: AbortSignal;
storedValuesLengthLimit: number;
distinctFieldsAbortThreshold?: number;
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 the naming, one thing we could do is add a description here if we feel the name doesn't fully encapsulate it's usage and intention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea!

@paula-stacho paula-stacho merged commit 7bf5720 into main Feb 19, 2025
20 checks passed
@paula-stacho paula-stacho deleted the COMPASS-8905 branch February 19, 2025 09:01
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.

2 participants