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

samples: Add snippets for sum and avg #480

Merged
merged 8 commits into from
Oct 3, 2023

Conversation

jlara310
Copy link
Contributor

Add sample snippets for sum and avg aggregation queries.

Two of these snippets depend on the following index:

indexes:

  • kind: Task
    properties:
    • name: done
    • name: hours

I added samples/snippets/index.yaml to track the indexes required for the samples.

Fixes #479 🦕

@jlara310 jlara310 requested review from a team as code owners September 13, 2023 20:35
@snippet-bot
Copy link

snippet-bot bot commented Sep 13, 2023

Here is the summary of changes.

You are about to add 5 region tags.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: datastore Issues related to the googleapis/python-datastore API. samples Issues that are directly related to samples. labels Sep 13, 2023
@daniel-sanche
Copy link
Contributor

We discussed this offline, but please see if you can modify the index as part of the test setup

Let me know if you need any help with this

@jlara310
Copy link
Contributor Author

@daniel-sanche

I added a test fixture to set up indexes but it's getting a permission error:

google.api_core.exceptions.PermissionDenied: 403 The caller does not have permission

@daniel-sanche
Copy link
Contributor

daniel-sanche commented Sep 19, 2023

@daniel-sanche

I added a test fixture to set up indexes but it's getting a permission error:

google.api_core.exceptions.PermissionDenied: 403 The caller does not have permission

Hmm, that's strange, because the service account has IndexAdmin permissions. And I can also see an audit log that looks like the operation was successful, and I see an index that looks like the one you created on datastore
image

I wonder what would happen if you removed the result() from admin_client.create_index(request).result()? It looks like it uses a separate rpc to get the operation status, I wonder if it's possible that is the one that is raising 403?

Or you could try catching the error to silence it for now, to see if the index you need was created properly

@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Sep 20, 2023
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels Sep 20, 2023
@jlara310
Copy link
Contributor Author

You're right. The IndexAdmin role does not have permissions for operations. Removed the code that tries to wait for the operation.

@jlara310
Copy link
Contributor Author

Only the Python 3.10 test failed. Perhaps the IndexAdmin role is missing for that project?

@daniel-sanche
Copy link
Contributor

thanks, I opened a request to add the missing permission to that project

@daniel-sanche daniel-sanche added the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 20, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 20, 2023
@daniel-sanche daniel-sanche added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 25, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 25, 2023
@daniel-sanche daniel-sanche added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 25, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 25, 2023
@daniel-sanche
Copy link
Contributor

@jlara310 Ok, the permissions have been added and the tests are now passing!

task3["hours"] = 1

tasks = [task1, task2, task3]
client.put_multi(tasks)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do datastore samples typically include this set-up part?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, as part of making samples copy-paste-runnable. This matches the style of the Java and Python count() samples, for example.

# Set up sample entities
task1 = datastore.Entity(client.key("Task", 4871162878230528))
task2 = datastore.Entity(client.key("Task", 5069834475798528))
task3 = datastore.Entity(client.key("Task", 5081353846521856))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any significance to these numbers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are randomly generated IDs from the Datastore console. A previous example used task1, task2, task3, but monotonically increasing IDs are an anti-pattern.

Copy link
Contributor

Choose a reason for hiding this comment

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

it might be worth either mentioning that in a comment, or even using random.random() to generate them in the sample if that would make sense in the context of the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Updated the code to let client generate the IDs and added a comment about it.

@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Sep 27, 2023
@daniel-sanche daniel-sanche added the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 27, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 27, 2023
@daniel-sanche daniel-sanche merged commit 4f91b51 into googleapis:main Oct 3, 2023
23 checks passed
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 googleapis/python-datastore API. samples Issues that are directly related to samples. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs: Add sample snippets for sum and avg aggregations.
3 participants