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

Support expressions in selectattr/rejectattr again, more efficiently #450

Merged
merged 3 commits into from
Jun 5, 2020

Conversation

tectonic8
Copy link
Contributor

Redoes the changes made (and reverted) in #238 to support full expressions as the first argument to the selectattr filter. The previous approach caused performance issues because of the generation of a unique expression for each value processed in the sequence. These expressions all get saved in the context (code), making iterating through resolved expressions and even just checking membership in the set (code) unwieldy.

This PR uses the same expression string for processing each value and removes it from the context after we've finished iterating through the loop. Doing so not only avoids the bloated context issue, but also allows us to take advantage of caching in the TreeStore when parsing the expression string into a TreeValueExpression.

Locally testing this implementation on very large lists (of 10,000 elements) showed that performance is the same, if not slightly faster, than the current master branch implementation that only supports chained attributes, and an order of magnitude faster than the #238 implementation with unique expressions.

@tectonic8 tectonic8 requested a review from mattcoley June 5, 2020 13:29
Copy link
Collaborator

@mattcoley mattcoley left a comment

Choose a reason for hiding this comment

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

Nice work 👍 Let's make sure to test this in QA quite a bit after this is merged in.

@tectonic8 tectonic8 merged commit e5ee648 into master Jun 5, 2020
@tectonic8 tectonic8 deleted the select-attr-expressions branch June 5, 2020 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants