Add concepts and concrete types for BlockZipperJoinImpl
#1625
Conversation
| if (allBlocksWereFilled) { | ||
| blockStatus = BlockStatus::allFilled; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Here, self is swapped with this
| AD_CORRECTNESS_CHECK(!lessThan_(lastProcessedElement, | ||
| std::as_const(blocks.front()).back())); | ||
| AD_CORRECTNESS_CHECK( | ||
| !lessThan_(lastProcessedElement, blocks.front().back())); |
There was a problem hiding this comment.
Here as_const is dropped. This presumably works because of recent fixes to the IdTable class.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1625 +/- ##
=======================================
Coverage 89.37% 89.38%
=======================================
Files 375 375
Lines 36107 36107
Branches 4077 4077
=======================================
+ Hits 32272 32274 +2
+ Misses 2518 2516 -2
Partials 1317 1317 ☔ View full report in Codecov by Sentry. |
| @@ -1228,7 +1253,8 @@ struct BlockZipperJoinImpl { | |||
| // `side.undefBlocks_` and skipped for subsequent processing. The first block | |||
| // containing defined values is split and the defined part is stored in | |||
| // `side.currentBlocks_`. | |||
| void findFirstBlockWithoutUndef(auto& side) { | |||
| void findFirstBlockWithoutUndef( | |||
| ad_utility::SameAsAny<LeftSide, RightSide> auto& side) { | |||
There was a problem hiding this comment.
The only concepts you are using are SameAsAny<LeftSide, RightSide> and SameAsAny<CurrentBlocksLeft, CurrentBlocksRight>.
Can we explore if we can remove some of the redundancy?
My first approach is: With concepts it doesn't work, they can't be local, you cant shorten their names, etc.
So a syntax could be to have a static constexpr bool and then write
template <typename T> requires LeftOrRightSide<T>
But that is not at all shorter....
If you want to....
Bad word Bad word Bad word...
** MACRO **
(don't forget to Undef it).
But maybe this all isn't worth it.
| // We can't define aliases for these concepts, so we use macros instead. | ||
| #define Side SameAsAny<LeftSide, RightSide> auto | ||
| #define Blocks SameAsAny<LeftBlocks, RightBlocks> auto |
There was a problem hiding this comment.
To be ultra safe,
pleas do an #if defined(Side) || defined(Blocks) ERROR(...) to guarantee that we don't accidentally overwrite any macros and introduce bugs that are impossibly to trace....
Conformance check passed ✅No test result changes. |
|
joka921
changed the title
BlockZipperJoinImplBlockZipperJoinImpl
This class (which is the central implementation of the lazy merge join) previously had a lot of
const auto&etc. parameters, which made the (already rather complex) code harder to reason about. This PR adds concrete types wherever possible, and constrains the other parameters using concepts.