-
Notifications
You must be signed in to change notification settings - Fork 73
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
Projections follow up #454
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ API Documentation | |
lifecycle | ||
partition | ||
predicate | ||
projection | ||
proxy/modules | ||
security | ||
serialization | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
Projection | ||
========== | ||
|
||
.. automodule:: hazelcast.projection |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
import hazelcast | ||
|
||
from hazelcast.core import HazelcastJsonValue | ||
from hazelcast.predicate import less_or_equal | ||
from hazelcast.projection import single_attribute, multi_attribute | ||
|
||
client = hazelcast.HazelcastClient() | ||
|
||
people = client.get_map("people").blocking() | ||
|
||
people.put_all( | ||
{ | ||
1: HazelcastJsonValue({"name": "Philip", "age": 46}), | ||
2: HazelcastJsonValue({"name": "Elizabeth", "age": 44}), | ||
3: HazelcastJsonValue({"name": "Henry", "age": 13}), | ||
4: HazelcastJsonValue({"name": "Paige", "age": 15}), | ||
} | ||
) | ||
|
||
names = people.project(single_attribute("name")) | ||
print("Names of the people are %s." % names) | ||
|
||
children_names = people.project(single_attribute("name"), less_or_equal("age", 18)) | ||
print("Names of the children are %s." % children_names) | ||
|
||
names_and_ages = people.project(multi_attribute("name", "age")) | ||
print("Names and ages of the people are %s." % names_and_ages) | ||
|
||
client.shutdown() |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,7 +48,7 @@ def get_class_id(self): | |
|
||
|
||
class _MultiAttributeProjection(_AbstractProjection): | ||
def __init__(self, *attribute_paths): | ||
def __init__(self, attribute_paths): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about adding types to new classes and functions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have added type hints to the projections module. I didn't type-hint the code that is coming from the superclass (IdentifiedDataSerializable), should I add type hints to those methods too (like get_class_id)? Also, I question. We have docstrings that define types for public methods, should we define type hints for them too? I have added type hints to them but I can remove them if you want There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to this SO discussion, it is possible to configure sphinx to recognize types, so writing in docstrings is probably not necessary: https://stackoverflow.com/questions/40071573/python-3-sphinx-doesnt-show-type-hints-correctly But I think we should keep them until we are sure about that. IMO adding types to only functions/classes added/changed in this PR is OK. We can tackle with adding types to others in a separate PR. I think types in the base class are used for derived classes, but since |
||
if not attribute_paths: | ||
raise ValueError("Specify at least one attribute path") | ||
|
||
|
@@ -81,7 +81,7 @@ def single_attribute(attribute_path): | |
|
||
Returns: | ||
Projection[any]: A projection that extracts the value of the given | ||
attribute path. | ||
attribute path. | ||
""" | ||
return _SingleAttributeProjection(attribute_path) | ||
|
||
|
@@ -95,16 +95,16 @@ def multi_attribute(*attribute_paths): | |
|
||
Returns: | ||
Projection[list]: A projection that extracts the values of the given | ||
attribute paths. | ||
attribute paths. | ||
""" | ||
return _MultiAttributeProjection(*attribute_paths) | ||
return _MultiAttributeProjection(attribute_paths) | ||
|
||
|
||
def identity(): | ||
"""Creates a projection that does no transformation. | ||
|
||
Returns: | ||
Projection[hazelcast.core.MapEntry]: A projection that does no | ||
transformation. | ||
transformation. | ||
""" | ||
return _IdentityProjection() |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,7 +41,7 @@ | |
from hazelcast.core import HazelcastJsonValue | ||
from hazelcast.config import IndexType, IntType | ||
from hazelcast.errors import HazelcastError | ||
from hazelcast.predicate import greater_or_equal, less_or_equal, sql, between | ||
from hazelcast.predicate import greater_or_equal, less_or_equal, sql, paging, true | ||
from hazelcast.proxy.map import EntryEventType | ||
from hazelcast.serialization.api import IdentifiedDataSerializable | ||
from hazelcast.six.moves import range | ||
|
@@ -796,6 +796,14 @@ def setUp(self): | |
def tearDown(self): | ||
self.map.destroy() | ||
|
||
def test_aggregate_with_none_aggregator(self): | ||
with self.assertRaises(AssertionError): | ||
self.map.aggregate(None) | ||
Comment on lines
+800
to
+801
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can be simplified as: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I personally prefer this style, and try to use it like this across the codebase. Advantages:
And IMHO, this one is more readable |
||
|
||
def test_aggregate_with_paging_predicate(self): | ||
with self.assertRaises(AssertionError): | ||
self.map.aggregate(int_avg("foo"), paging(true(), 10)) | ||
|
||
def test_int_average(self): | ||
average = self.map.aggregate(int_avg()) | ||
self.assertEqual(24.5, average) | ||
|
@@ -1034,23 +1042,32 @@ def setUp(self): | |
def tearDown(self): | ||
self.map.destroy() | ||
|
||
def test_project_with_none_projection(self): | ||
with self.assertRaises(AssertionError): | ||
self.map.project(None) | ||
|
||
def test_project_with_paging_predicate(self): | ||
with self.assertRaises(AssertionError): | ||
self.map.project(single_attribute("foo"), paging(true(), 10)) | ||
|
||
def test_single_attribute(self): | ||
attribute = self.map.project(single_attribute("attr1")) | ||
six.assertCountEqual(self, [4, 1], attribute) | ||
attributes = self.map.project(single_attribute("attr1")) | ||
six.assertCountEqual(self, [1, 4], attributes) | ||
|
||
def test_single_attribute_with_predicate(self): | ||
attribute = self.map.project(single_attribute("attr1"), greater_or_equal("attr1", 4)) | ||
self.assertEqual([4], attribute) | ||
attributes = self.map.project(single_attribute("attr1"), greater_or_equal("attr1", 4)) | ||
six.assertCountEqual(self, [4], attributes) | ||
|
||
def test_multi_attribute(self): | ||
attributes = self.map.project(multi_attribute("attr1", "attr2")) | ||
six.assertCountEqual(self, [[4, 5], [1, 2]], attributes) | ||
six.assertCountEqual(self, [[1, 2], [4, 5]], attributes) | ||
|
||
def test_multi_attribute_with_predicate(self): | ||
attributes = self.map.project( | ||
multi_attribute("attr1", "attr2"), greater_or_equal("attr2", 3) | ||
multi_attribute("attr1", "attr2"), | ||
greater_or_equal("attr2", 3), | ||
) | ||
self.assertEqual([[4, 5]], attributes) | ||
six.assertCountEqual(self, [[4, 5]], attributes) | ||
|
||
def test_identity(self): | ||
attributes = self.map.project(identity()) | ||
|
@@ -1065,6 +1082,8 @@ def test_identity(self): | |
|
||
def test_identity_with_predicate(self): | ||
attributes = self.map.project(identity(), greater_or_equal("attr2", 3)) | ||
self.assertEqual( | ||
HazelcastJsonValue('{"attr1": 4, "attr2": 5, "attr3": 6}'), attributes[0].value | ||
six.assertCountEqual( | ||
self, | ||
[HazelcastJsonValue('{"attr1": 4, "attr2": 5, "attr3": 6}')], | ||
[attribute.value for attribute in attributes], | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
import unittest | ||
|
||
from hazelcast.projection import single_attribute, multi_attribute | ||
|
||
|
||
class ProjectionsInvalidInputTest(unittest.TestCase): | ||
def test_single_attribute_with_any_operator(self): | ||
with self.assertRaises(ValueError): | ||
single_attribute("foo[any]") | ||
|
||
def test_single_attribute_with_empty_path(self): | ||
with self.assertRaises(ValueError): | ||
single_attribute("") | ||
|
||
def test_multi_attribute_with_no_paths(self): | ||
with self.assertRaises(ValueError): | ||
multi_attribute() | ||
|
||
def test_multi_attribute_with_any_operator(self): | ||
with self.assertRaises(ValueError): | ||
multi_attribute("valid", "invalid[any]") | ||
|
||
def test_multi_attribute_with_empty_path(self): | ||
with self.assertRaises(ValueError): | ||
multi_attribute("valid", "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about replacing
single_attribute
andmulti_attribute
with justattribute
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But they are two distinct things with distinct return types. One returns
list[any]
, the otherlist[list[any]]
when used with project method.Also, I find such codes like the below a bit weird in the implementation. IMHO, a separation is better than this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we make it just
attribute
, the return type can belist[list[any]]
. I think it's not weird at all to distinguish between_SingleAttribute
and_MultiAttribute
depending on the number of attributes. Those two are implementation details, the user doesn't need to know about them. (I didn't check that, but I guess they are different in the protocol as well, not sure about the reason of that decision)