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

Fix #83: Add 'exclude_from_indexes' method to Entity. #330

Merged
merged 3 commits into from
Nov 4, 2014
Merged

Fix #83: Add 'exclude_from_indexes' method to Entity. #330

merged 3 commits into from
Nov 4, 2014

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Nov 3, 2014

Set it via ctor argument.

Pass it to Connection.save_entity.

Fields in the sequence will have the indexed field set False in the
corrsponding protobuf.

Fixes #83.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 79b4d5a on tseaver:83-expose_entity_indexed into f33e557 on GoogleCloudPlatform:master.

@@ -387,6 +388,9 @@ def save_entity(self, dataset_id, key_pb, properties):

:type properties: dict
:param properties: The properties to store on the entity.

:type exclude_from_indexes: sequence of str

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@dhermes
Copy link
Contributor

dhermes commented Nov 3, 2014

@silvolu Do you guys support this in gcloud-node? If yes, is there a reason there aren't regression tests for it?

For example, a simple test like:

e1 = dataset.entity(kind='Test')
e1['name'] = 'val'
e1.save()

e2 = dataset.entity(kind='Test')
e2['name'] = 'val'
e2._exclude_from_indexes = set(['name'])
e2.save()

query = dataset.query('Test').filter('name =', 'val').limit(2)
results = query.fetch()

# Make sure len(results) == 1

@tseaver Shouldn't we have support for this in the Dataset convenience method for creating an Entity?

@tseaver
Copy link
Contributor Author

tseaver commented Nov 3, 2014

Ugh, yet more reasons to detest those "convenience" APIs. ;)

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 6b8f52a on tseaver:83-expose_entity_indexed into f33e557 on GoogleCloudPlatform:master.

@dhermes
Copy link
Contributor

dhermes commented Nov 3, 2014

Indeed. We should work hard to have a single entry point, but one that is very nice.

The "old way", i.e. within the App Engine environment, the dataset ID is always known, so users can just skip right to creating entities. Maybe we could do similar here by reading an env var or modifying a constant on a module or something else.

@silvolu
Copy link
Contributor

silvolu commented Nov 4, 2014

@dhermes we support it in a different way (you set it on the attribute), and we have unit tests only for it, given that the essence of it is setting the correct value on the pb sent over.
Also, I guess a regression test would have to test that the query is failing because the index doesn't exist?

@dhermes
Copy link
Contributor

dhermes commented Nov 4, 2014

@silvolu An equality query on a single property does not require an index.

My test code above should return a single entry (# Make sure len(results) == 1) even though two entries match the query; this is because one of those two has the field unindexed. This is makes sure that the way we set the pb's is the correct one.

FWIW I'm a bit worried about more regression tests as they already take a fair bit of time to run. Maybe we could have a simple suite and an expanded suite that runs only when releases are cut.

@silvolu
Copy link
Contributor

silvolu commented Nov 4, 2014

@dhermes I seem to constantly forget how indexes work :)
I think it's fine if we don't test this against the prod api.
In general though I'd be more inclined to run a more complete suite of functional tests upon merges, so that we can catch potential breaks in a timely fashion and fix the cause.

@dhermes
Copy link
Contributor

dhermes commented Nov 4, 2014

Everything looks good except for str vs. unicode question

@tseaver
Copy link
Contributor Author

tseaver commented Nov 4, 2014

@dhermes 4c4f891 updates the docstring entries for exclude_from_indexes to avoid mandating the type of the field names.

@dhermes
Copy link
Contributor

dhermes commented Nov 4, 2014

@tseaver Thanks for making the change. LGTM.

I'm still unclear if using unicode for kind names will cause something to break either in our library or on the GCD server.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 4c4f891 on tseaver:83-expose_entity_indexed into f33e557 on GoogleCloudPlatform:master.

Set it via ctor argument.

Pass it to 'Connection.save_entity'.

Fields in the sequence will have the 'indexed' field set False in the
corrsponding protobuf.

Fixes #83.
@tseaver
Copy link
Contributor Author

tseaver commented Nov 4, 2014

Rebased to resolve conflict from #331.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 5762cbf on tseaver:83-expose_entity_indexed into abe3d7c on GoogleCloudPlatform:master.

@dhermes
Copy link
Contributor

dhermes commented Nov 4, 2014

Still LGTM. Still unclear about using unicode kinds.

tseaver added a commit that referenced this pull request Nov 4, 2014
Fix #83:  Add 'exclude_from_indexes' method to Entity.
@tseaver tseaver merged commit df05a0e into googleapis:master Nov 4, 2014
@tseaver tseaver deleted the 83-expose_entity_indexed branch November 4, 2014 18:33
@dhermes dhermes added the api: datastore Issues related to the Datastore API. label Dec 31, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the Datastore API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gcloud.datastore should allow explicit specification of whether the field is indexed
4 participants