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

Resolves #135: DocLayer is not able to handle sparse arrays correctly. #165

Conversation

dongxinEric
Copy link
Contributor

Previously, the null imputing logic used when doing path expansion is not handling the sparse array at all. This PR mainly resolves that problem by adding a third case when it impute a null value. This PR also fixed several bugs in MongoModel.

Regarding to the sparse array: right now in insertElementRecursive(), DocLayer does NOT try to store arrays with null values as sparse arrays, which is kind of against what it attempts to do later when doing update. Filed #164 tracking that.

Xin Dong added 4 commits April 15, 2019 13:13
- {} should NOT match anything other than an empty dictionary in some cases
- indexes should be checked after all update operators
@dongxinEric dongxinEric requested a review from apkar April 16, 2019 23:55
@dongxinEric dongxinEric self-assigned this Apr 16, 2019
Copy link
Contributor

@apkar apkar 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 (r < 0.1) and generator_options.test_nulls:
# val = None
# val = None
Copy link
Contributor

Choose a reason for hiding this comment

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

With this patch, is it time to re-enable None value tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do in the PR that adds deletion tests

@apkar apkar merged commit fb3b7a7 into FoundationDB:master Apr 23, 2019
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