Skip to content

175 descriptor set optimizations batch inserts#209

Merged
s-gobriel merged 10 commits intodevelopfrom
175-descriptor-set-optimizations-batch-inserts
Aug 29, 2024
Merged

175 descriptor set optimizations batch inserts#209
s-gobriel merged 10 commits intodevelopfrom
175-descriptor-set-optimizations-batch-inserts

Conversation

@keirafadams
Copy link
Copy Markdown
Contributor

Initial development done, still some refinement and test updates to be done, but ready for initial review.

@github-actions
Copy link
Copy Markdown
Contributor

Target CPP Coverage: 64.3906%
Source CPP Coverage: 64.0043%

Target Python Coverage: 98.02%
Source Python Coverage: 98.02%

@keirafadams
Copy link
Copy Markdown
Contributor Author

@rolandoquesada Question for you: I'm assuming our "end to end" tests from python do not really exercise coverage outside of the client because they are running against the server?

@github-actions
Copy link
Copy Markdown
Contributor

Target CPP Coverage: 64.3906%
Source CPP Coverage: 64.0032%

Target Python Coverage: 98.02%
Source Python Coverage: 98.02%

@github-actions
Copy link
Copy Markdown
Contributor

Target CPP Coverage: 64.3906%
Source CPP Coverage: 63.9842%

Target Python Coverage: 98.02%
Source Python Coverage: 98.02%

@github-actions
Copy link
Copy Markdown
Contributor

Target CPP Coverage: 64.3906%
Source CPP Coverage: 63.9842%

Target Python Coverage: 98.02%
Source Python Coverage: 98.02%

@keirafadams
Copy link
Copy Markdown
Contributor Author

Alright folks, I think this is ready for a merge, however we have some nits around code coverage.

@keirafadams
Copy link
Copy Markdown
Contributor Author

@araghuna1 @s-gobriel any final updates/review comments?

araghuna1
araghuna1 previously approved these changes Aug 29, 2024
@s-gobriel
Copy link
Copy Markdown
Contributor

Very good work @ifadams on the batch inserts. I wonder if you think it is valuable to add a couple of c++ unit tests (in addition to the python tests you included for end-to-end to testing). These unittests can target the functionality to add in batches using the different engines and retrieve and check that the values are correct. but otherwise, I think this is good.

@keirafadams
Copy link
Copy Markdown
Contributor Author

Very good work @ifadams on the batch inserts. I wonder if you think it is valuable to add a couple of c++ unit tests (in addition to the python tests you included for end-to-end to testing). These unittests can target the functionality to add in batches using the different engines and retrieve and check that the values are correct. but otherwise, I think this is good.

Hey @s-gobriel, if you have concern about this, we can add in additional tests now, or if we want to get it merged immediately we should create an issue to immediately develop these tests as a task next quarter. I might suggest the latter, but I'm okay with either decision.

@s-gobriel
Copy link
Copy Markdown
Contributor

I understand. Let's add these tests in the next quarter, especially that the python test is covering the functionality

@github-actions
Copy link
Copy Markdown
Contributor

Target CPP Coverage: 64.3906%
Source CPP Coverage: 63.9842%

Target Python Coverage: 98.02%
Source Python Coverage: 98.02%

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