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

Add the constructor to in_predicate where we can also accept a vector [API-551] #890

Merged
merged 2 commits into from
Jun 30, 2021

Conversation

ihsandemir
Copy link
Collaborator

Added the constructor to in_predicate where we can also accept a vector of keys/values for the query. Also added a new test IssueTest::issue_888 and updated the ClientMapTest::testValuesWithPredicate to use a vector of keys. The query example should also work with this fix.

fixes #888

…ctor of keys/values for the query. Also added a new test `IssueTest::issue_888` and updated the `ClientMapTest::testValuesWithPredicate` to use a vectory of keys. The query example should also work with this fix.

fixes hazelcast#888
@ihsandemir ihsandemir added this to the 4.1.2 milestone Jun 21, 2021
@ihsandemir ihsandemir self-assigned this Jun 21, 2021
@devOpsHazelcast
Copy link
Contributor

Windows test PASSed.

@devOpsHazelcast
Copy link
Contributor

Linux test PASSed.

@@ -64,6 +64,15 @@ namespace hazelcast {
out_stream.write<int32_t>(static_cast<int32_t>(sizeof...(values)));
out_stream.write_objects(values...);
}

template<typename T>
multi_predicate(const std::string attribute_name, hazelcast_client &client, const std::vector<T> &values) : base_predicate(client) {
Copy link

Choose a reason for hiding this comment

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

Why is the hazelcast_client needed to create a predicate? IMO it would be great to get rid of it if possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

unfortunately, we need it to access the serialization service. I also do not like it but i had to do it due to the fact that i needed to prepare the binary during construction.

Copy link

Choose a reason for hiding this comment

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

I see. I wish we could defer binary construction. Taking a client as a parameter has more problems, like would passing a different to multi_predicate than the one calling the actual query cause problems? But I guess this is not something that can be changed in this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Passing a different client, yes, that may cause a problem if serialization configs are different. Alternative was to use the client as the factory method to create the in_predicate but it again has the same problem if the user would use a client predicate with the other client. I am open to any better solution but could not find one as of yet. Yes, we can open another issue to discuss this topic.

@degerhz degerhz changed the title Add the constructor to in_predicate where we can also accept a vector Add the constructor to in_predicate where we can also accept a vector [API-551] Jun 30, 2021
@devOpsHazelcast
Copy link
Contributor

Windows test PASSed.

@devOpsHazelcast
Copy link
Contributor

Linux test PASSed.

@ihsandemir ihsandemir merged commit a97de79 into hazelcast:master Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

strange in_predicate behaviour
4 participants