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

PHPC-2478: Implement API for bulkWrite command #1763

Closed
wants to merge 13 commits into from

Conversation

jmikola
Copy link
Member

@jmikola jmikola commented Dec 10, 2024

@jmikola jmikola force-pushed the 1.x-phpc-2478 branch 3 times, most recently from 7a7b316 to b1353e6 Compare January 7, 2025 19:07
@jmikola jmikola force-pushed the 1.x-phpc-2478 branch 2 times, most recently from cc280df to d5d3c10 Compare January 16, 2025 19:35
Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

Initial feedback on the code - I haven't yet gone through all the tests.

PHONGO_BSON_INIT_STATE(state);
state.map.root.type = PHONGO_TYPEMAP_NATIVE_ARRAY;

/* TODO: Instead of converting the entire document, iterate BSON to obtain
Copy link
Member

Choose a reason for hiding this comment

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

Yep, using bson_iter_init_find and bson_iter_value is probably significantly faster than walking the entire BSON structure 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Created PHPC-2516 to track this.

return;
}

if (php_array_existsc(zoptions, "bypassDocumentValidation")) {
Copy link
Member

Choose a reason for hiding this comment

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

We do have some macros that we use for queries - would it make sense to make these macros universal? I'm thinking of something like this:

PHONGO_EXTRACT_OPTION_BOOL(zoptions, "bypassDocumentValidation", intern->bypass)

If so, I can create a ticket for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think there is sufficient overlap with the macros in Query.c to make that worthwhile. In Query, the parsed options go directly to a bson_t. Here, they're stashed on the object struct. For BulkWriteCommand's operation methods, they're going to be assigned via distinct setter methods (similar to most of the option handling in ClientEncryption.c).

@jmikola jmikola force-pushed the 1.x-phpc-2478 branch 2 times, most recently from 7542cba to 3a45310 Compare January 28, 2025 18:30
Per CDRIVER-5843, libmongoc does not consistently populate this field. It also isn't required by the spec, so omit it for now.
@jmikola
Copy link
Member Author

jmikola commented Feb 18, 2025

Superseded by #1790, which is rebased on v2.x.

@jmikola jmikola closed this Feb 18, 2025
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.

3 participants