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

Filter out diff deltas that don't match the spatial filter. #461

Merged
merged 2 commits into from
Jul 21, 2021

Conversation

olsen232
Copy link
Collaborator

@olsen232 olsen232 commented Jul 20, 2021

Description

Feature deltas are only relevant to the user if either the old or the new version of the feature is inside the spatial filter. The exception to this rule is local working-copy edits, which are always relevant to the user. This PR filters out irrelevant deltas that do not match the spatial filter.

Related links:

#456

@olsen232 olsen232 requested review from craigds and rcoup July 20, 2021 04:20
Copy link
Member

@rcoup rcoup left a comment

Choose a reason for hiding this comment

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

Spatial filtered diff output is not yet properly streamed - every feature is loaded before the first feature is output. (In contrast to diffs where there is no spatial filter, where each feature is loaded just in time). This should be fixed at the very least for json-lines output, which is where streaming is most beneficial.

Is there a reason that yielding matching features one at a time isn't straightforward? Or simply to get it going?

@olsen232
Copy link
Collaborator Author

Spatial filtered diff output is not yet properly streamed - every feature is loaded before the first feature is output. (In contrast to diffs where there is no spatial filter, where each feature is loaded just in time). This should be fixed at the very least for json-lines output, which is where streaming is most beneficial.

Is there a reason that yielding matching features one at a time isn't straightforward? Or simply to get it going?

I've done it now, so, it's not impossible - it just took a bit of restructuring. As the code was, filtering feature deltas immediately before they were output was not possible, since at that point, there was no way to tell which deltas were from commits and which were from the WC (and the two sources are filtered differently). However, filtering feature deltas at the point where we did have this information was too soon - this information was lost before the first feature was output, so if we do spatial filtering at that point, it means loading every feature before outputting the first one.

The fix is just to propagate which features are from commits and which features are from the WC.

Also added a warning to kart create-patch so that TODO is done now too - we can always revisit this if we decide kart create-patch should not have be affected by spatial filtering at all.

@olsen232 olsen232 merged commit b0c4f5a into master Jul 21, 2021
@olsen232 olsen232 deleted the spatial-filter-3 branch July 21, 2021 00:36
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