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

optimize calendar search query #39741

Merged
merged 2 commits into from Aug 9, 2023

Conversation

jmcclelland
Copy link
Contributor

@jmcclelland jmcclelland commented Aug 7, 2023

Summary

By moving the classification clause to the outer query it returns more quickly and efficiently, avoiding potential time out when viewing calendars with large numbers of items.

Checklist

@kesselb
Copy link
Contributor

kesselb commented Aug 7, 2023

Thanks for your pull request 👍

The failure looks related:

 There was 1 failure:

1) OCA\DAV\Tests\unit\CalDAV\CalDavBackendTest::testSearch with data set #1 (true, array(array(DateTime Object (...), DateTime Object (...))), 2)
Failed asserting that actual size 4 matches expected size 2.

/home/runner/work/server/server/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php:919

@kesselb kesselb added this to the Nextcloud 28 milestone Aug 7, 2023
@jmcclelland
Copy link
Contributor Author

Thanks @kesselb , it does seem related. It seems like the test inserts four calendar items, only two of which are public. And it should only return 2 items (the public ones) but with my change it is returning 4 of them.

I'm not sure why. @ChristophWurst - do you have any ideas on why it woudl fail this test?

@jmcclelland
Copy link
Contributor Author

I think I found the problem. I suspect the where() call clobbered the earlier andWhere() call. @kesselb - is it possible to re-test with the latest change? Thank you!

@kesselb
Copy link
Contributor

kesselb commented Aug 7, 2023

I suspect the where() call clobbered the earlier andWhere() call.

Well spotted, where will overwrite/reset andWhere.

is it possible to re-test with the latest change?

Looks good now!

DCO

Can you sign off your commits?

git rebase HEAD~2 --signoff
git push --force-with-lease origin optimize-cal-query

@kesselb kesselb requested a review from st3iny August 7, 2023 18:37
@kesselb kesselb added 3. to review Waiting for reviews performance 🚀 and removed 2. developing Work in progress labels Aug 7, 2023
see nextcloud/calendar#4758

Signed-off-by: Jamie McClelland <jm@mayfirst.org>
Signed-off-by: Jamie McClelland <jm@mayfirst.org>
@jmcclelland
Copy link
Contributor Author

Thanks for the help and feedback. Let me know if there's is anything else I should do.

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Thanks a lot

Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@st3iny st3iny added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 8, 2023
@st3iny
Copy link
Member

st3iny commented Aug 8, 2023

Drone failure doesn't seem to be related (acceptance-app-files).

@solracsf
Copy link
Member

solracsf commented Aug 8, 2023

Seems a good candidate to be backported 👍

@ChristophWurst
Copy link
Member

/backport to stable27

@ChristophWurst
Copy link
Member

/backport to stable26

@ChristophWurst
Copy link
Member

/backport to stable25

@ChristophWurst ChristophWurst merged commit 42d1568 into nextcloud:master Aug 9, 2023
33 of 37 checks passed
@welcome
Copy link

welcome bot commented Aug 9, 2023

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@backportbot-nextcloud
Copy link

The backport to stable27 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable27
git pull origin stable27

# Create the new backport branch
git checkout -b fix/foo-stable27

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable27

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

@backportbot-nextcloud
Copy link

The backport to stable26 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable26
git pull origin stable26

# Create the new backport branch
git checkout -b fix/foo-stable26

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable26

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

@backportbot-nextcloud
Copy link

The backport to stable25 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable25
git pull origin stable25

# Create the new backport branch
git checkout -b fix/foo-stable25

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable25

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

@ChristophWurst
Copy link
Member

/backport to stable27

@ChristophWurst
Copy link
Member

/backport to stable26

@ChristophWurst
Copy link
Member

/backport to stable25

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement performance 🚀
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants