Skip to content

Conversation

@ThierryBerger
Copy link
Contributor

@ThierryBerger ThierryBerger commented Jun 25, 2025

As #105 is labeled good first issue, I took my chances, I look forward to feedbacks to help me bring it to completion.

background

Current state

I added try_bulk_load_cdt_stable and try_bulk_load_cdt functions, as well as a test.

Existing try_bulk_load*s are affected as their functions now use the try_add_constraints., but their behaviour is kept to panicking on conflicting edges encountered.

This serves as discussion starters, I imagine this comes at a performance price and will need 2 different functions.
Copy link
Owner

@Stoeoef Stoeoef left a comment

Choose a reason for hiding this comment

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

Thank you so much for opening this PR!

I've done a cursory review, most of this already looks pretty good.

The performance impact of using try_add_constraint should be negligible - no worries there.

However, we'll need to keep the old panic behavior for bulk_load_cdt - could you take a look into that? Thank you!

@Stoeoef
Copy link
Owner

Stoeoef commented Jun 25, 2025

There are also two clippy lints.

Please add an appropriate [ignore] for the "error: very complex type used. Consider factoring parts into type definitions" lint - I'll see if this can be simplified (though I don't think so...)

@ThierryBerger ThierryBerger force-pushed the 105-faillible-bulk-load branch from 674d3b4 to fb9e2f7 Compare June 26, 2025 07:48
Reinstate the panic behaviour

fixup docs
@ThierryBerger ThierryBerger force-pushed the 105-faillible-bulk-load branch from fb9e2f7 to 8785ab8 Compare June 26, 2025 07:48
@ThierryBerger ThierryBerger requested a review from Stoeoef June 26, 2025 07:53
Copy link
Owner

@Stoeoef Stoeoef left a comment

Choose a reason for hiding this comment

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

All looks good to me. 💯
Thank you for the contribution - merging in!

I'll have a look at [try_]bulk_load_cdt_stable - maybe it can easily be improved? - and will create a new Spade version once that's done.

@Stoeoef Stoeoef merged commit 699f446 into Stoeoef:master Jun 27, 2025
5 checks passed
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.

Implement CDT bulk load method that don't panic if constraints intersect

2 participants