[c++] Handling edge cases for C++ re-indexer #2098
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bkmartinjr
reviewed
Jan 31, 2024
bkmartinjr
reviewed
Jan 31, 2024
bkmartinjr
reviewed
Jan 31, 2024
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2098 +/- ##
==========================================
+ Coverage 76.72% 80.06% +3.33%
==========================================
Files 136 87 -49
Lines 10672 8040 -2632
Branches 207 0 -207
==========================================
- Hits 8188 6437 -1751
+ Misses 2386 1603 -783
+ Partials 98 0 -98
Flags with carried forward coverage won't be shown. Click here to find out more.
|
beroy
force-pushed
the
edgecases_for_reindexer
branch
from
7450694
to
a00b3d1
Compare
bkmartinjr
reviewed
Jan 31, 2024
beroy
force-pushed
the
edgecases_for_reindexer
branch
4 times, most recently
from
8b85b19
to
a0ac10c
Compare
|
@bkmartinjr done with fixing both issue and now waiting for CI |
bkmartinjr
reviewed
Feb 1, 2024
| if (size == 0) { | ||
| return; | ||
| } | ||
| if (threads == 0) { | ||
| throw std::runtime_error("Re-indexer thread count cannot be zero."); | ||
| } | ||
|
|
||
| LOG_DEBUG(fmt::format( |
There was a problem hiding this comment.
any reason to have duplicate logging here - same information is in both statements....
bkmartinjr
reviewed
Feb 1, 2024
| LOG_DEBUG(fmt::format( | ||
| "Lookup with thread concurrency {} on data size {}", | ||
| tiledb_thread_pool_->concurrency_level(), | ||
| size)); |
There was a problem hiding this comment.
nit: these two log lines are largely redundant as well
bkmartinjr
reviewed
Feb 1, 2024
bkmartinjr
approved these changes
Feb 1, 2024
beroy
force-pushed
the
edgecases_for_reindexer
branch
from
a0ac10c
to
0962ae8
Compare
beroy
force-pushed
the
edgecases_for_reindexer
branch
from
0962ae8
to
f6678fe
Compare
johnkerl
pushed a commit
that referenced
this pull request
Feb 1, 2024
johnkerl
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue and/or context:
Some of the edge case like negative size table and handling thread sizes smaller or equal to one could be handled better
Changes:
Notes for Reviewer: