Skip to content

Conversation

@Primevenn
Copy link
Contributor

@Primevenn Primevenn commented Apr 15, 2021

fix: support ordering by key for multi queries

Fixes #629

@Primevenn Primevenn requested a review from andrewsg as a code owner April 15, 2021 03:56
@Primevenn Primevenn requested a review from a team April 15, 2021 03:56
@product-auto-label product-auto-label bot added the api: datastore Issues related to the googleapis/python-ndb API. label Apr 15, 2021
@google-cla
Copy link

google-cla bot commented Apr 15, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no This human has *not* signed the Contributor License Agreement. label Apr 15, 2021
@Primevenn Primevenn force-pushed the fix/multiquery_order_by_key branch from f74f417 to f4410be Compare April 15, 2021 04:08
@google-cla
Copy link

google-cla bot commented Apr 15, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@Primevenn
Copy link
Contributor Author

@googlebot I signed it!

@google-cla google-cla bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Apr 15, 2021
@chrisrossi chrisrossi added the kokoro:run Add this label to force Kokoro to re-run the tests. label Apr 20, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Apr 20, 2021
@chrisrossi
Copy link
Contributor

@Primevenn This PR looks great. Thank you for submitting it.

In order to accept this PR we'll need unit test coverage for the new code. There are also some linting errors that can be cleaned up by running black. (nox -e blacken)

Once you can run nox locally and not get any errors, you should be about good to go.

@Primevenn Primevenn force-pushed the fix/multiquery_order_by_key branch from a4f175e to ed18867 Compare April 21, 2021 04:17
@Primevenn Primevenn changed the title fix - support ordering by key for multi queries fix: support ordering by key for multi queries Apr 21, 2021
@Primevenn
Copy link
Contributor Author

Primevenn commented Apr 21, 2021

@chrisrossi Thanks for the review!

As you suggested i've appended flat_path to the 2 lines under the if statement that handles ordering by Entity __ key __ and have moved the other if statements inside the else block.

These 2 extra if statements are necessary for ordering by keyProperty since helpers._get_value_from_value_pb() will return a Key object for key_value fields.

I've also added unit test coverage for these 2 scenarios and ran black.

@chrisrossi chrisrossi added the kokoro:run Add this label to force Kokoro to re-run the tests. label Apr 21, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Apr 21, 2021
Copy link
Contributor

@chrisrossi chrisrossi left a comment

Choose a reason for hiding this comment

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

Excellent. Thank you!

@chrisrossi chrisrossi requested a review from a team as a code owner May 4, 2021 20:30
@chrisrossi chrisrossi added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels May 5, 2021
@yoshi-kokoro yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels May 5, 2021
@chrisrossi chrisrossi merged commit 508d8cb into googleapis:master May 5, 2021
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-ndb API. cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Encountering issues with multiqueries ordered by key

3 participants