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

[API/feature?] make fids caching optional #32315

Merged
merged 1 commit into from
Nov 1, 2019
Merged

Conversation

roya0045
Copy link
Contributor

Description

This is a follow up PR to #31061 that enabled caching of fids per symbol during the counting procedure.

This PR aims to make fids caching optional, as it is not necessary to have the have the fids in most cases, especially until #30297 is merged.

This should prevent for useless memory usage.

If this is merged after 3.10 is published, backporting is highly recommended to prevent extra memory usage.

Checklist

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include Fixes #11111 at the bottom of the commit message
  • I have read the QGIS Coding Standards and this PR complies with them
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit
  • I have evaluated whether it is appropriate for this PR to be backported, backport requests are left as label or comment

@roya0045 roya0045 changed the title [API/feature?] optional fid caching [API/feature?] make fids caching optional Oct 18, 2019
@roya0045
Copy link
Contributor Author

@nyalldawson @m-kuhn I'm not sure if this is a feature or a preventive bugfix but this would be nice to have it in 3.10 in case some users have to do counting on big dataset.

@nyalldawson nyalldawson added the API API improvement only, no visible user interface changes label Oct 27, 2019
@nyalldawson nyalldawson added this to the 3.12.0 milestone Oct 27, 2019
@roya0045
Copy link
Contributor Author

@m-kuhn a quick review would be appreciated.

@roya0045
Copy link
Contributor Author

Backport would be preferable, especially early in this cycle.

Copy link
Member

@m-kuhn m-kuhn left a comment

Choose a reason for hiding this comment

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

Looks good to me, just some minor changes to wording.
And +1 for backporting.

python/core/auto_generated/qgsvectorlayer.sip.in Outdated Show resolved Hide resolved
python/core/auto_generated/qgsvectorlayer.sip.in Outdated Show resolved Hide resolved
@roya0045 roya0045 reopened this Oct 31, 2019
@roya0045
Copy link
Contributor Author

@m-kuhn Thanks for the review/suggestions. I have also made the same changes to the counter in order to have constant docs.

Copy link
Member

@m-kuhn m-kuhn 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

@roya0045
Copy link
Contributor Author

roya0045 commented Nov 1, 2019

@m-kuhn Did I need to do anything for the backporting? I assume a manual backport is only needed when automatic backporting fails.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API API improvement only, no visible user interface changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants