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

Sai/efficient union #561

Merged
merged 15 commits into from
May 16, 2024
Merged

Sai/efficient union #561

merged 15 commits into from
May 16, 2024

Conversation

saiskee
Copy link
Contributor

@saiskee saiskee commented May 8, 2024

Description

This makes Union more efficient by checking if the input set is a sorted set, and taking advantage of merging two sorted lists (O(n+m))

@saiskee saiskee marked this pull request as ready for review May 10, 2024 17:44

// Guarantees that when running Iter, Filter, or FilterOutAndCreateList, elements in the set will be processed in a
// sorted order by ResourceId. See ezkube.ResourceIdsCompare for the definition of ResourceId sorting.
IsSortedByResourceId() bool
Copy link
Member

Choose a reason for hiding this comment

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

This feels very very specific and kind of strange to have on the interface to be honest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need a way to guarantee that the resource set in Union is sorted for more efficient merging. I'm open to other ideas on how we can do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the need for knowing the set has been sorted by a specific type, but I don't like how this func is tied to implementation detail. I think making it generic with a name like SortedBy that uses/returns an enum value could make more sense. The enum would of course have to hold all known sorting types but this way it generalized, and the enum documents what all we sort by (yes that is implementation detail but abstracted).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Impmlemented this change @conradhanson

Copy link
Member

Choose a reason for hiding this comment

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

Don't these lists always have to be sorted? I actually think it's up to the caller to make sure they're sorted by the same thing. Union should just perform its operation.

Copy link
Member

@EItanya EItanya May 13, 2024

Choose a reason for hiding this comment

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

Then those are all sorted, also the other implementations could Union efficiently already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless two resource sets are sorted by different criteria, then we'd have to check the sorting criteria are the same. If not, fallback to the receiving set's sort method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@EItanya is there a change you want to see here? not sure if refactoring this and figuring out the perfect semantics is a blocker here.

Copy link
Member

Choose a reason for hiding this comment

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

You're right the semantics don't have to be perfect, but I think worrying about the sorting is exposing too many details. I would recommend removing this entirely and leaving it up to the caller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed IsSortedBy and use sorted set merging by default.

@saiskee saiskee requested a review from EItanya May 16, 2024 19:30
@soloio-bulldozer soloio-bulldozer bot merged commit 01eb817 into main May 16, 2024
3 checks passed
@soloio-bulldozer soloio-bulldozer bot deleted the sai/efficient-union branch May 16, 2024 21:40
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.

4 participants