Fix TANE-based algorithms #396
Open
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
Merged
iliya-b
force-pushed
the
tane-refactoring
branch
3 times, most recently
from
6aadb62 to
c396254
Compare
iliya-b
force-pushed
the
tane-refactoring
branch
3 times, most recently
from
e9b9c16 to
e319415
Compare
vs9h
reviewed
Nov 27, 2024
There was a problem hiding this comment.
I can't say that I fully remember the essence of the algorithm and can fully verify the changes.
Can you provide useful links? I would like to study in more detail what is happening in these methods.
Also, is it known how much these changes affect performance?
In general, I think the changes are adequate and most likely after I study in more detail what is happening here, it will be possible to merge.
| @@ -258,4 +249,4 @@ unsigned long long TaneCommon::ExecuteInternal() { | |||
|
|
|||
| } // namespace tane | |||
|
|
|||
| } // namespace algos | |||
| } // namespace algos | |||
| for (std::size_t rhs_index = vertex->GetRhsCandidates().find_first(); | ||
| rhs_index != boost::dynamic_bitset<>::npos; | ||
| rhs_index = vertex->GetRhsCandidates().find_next(rhs_index)) { | ||
| Vertical rhs = static_cast<Vertical>(*schema->GetColumn((int)rhs_index)); |
There was a problem hiding this comment.
nit: remove (int) cast, since we should pass std::size_t
Comment on lines
-80
to
+79
| // if we seek for exact FDs then SetInvalid | ||
| if (max_fd_error_ == 0 && max_ucc_error_ == 0) { | ||
| for (auto key_vertex : key_vertices) { | ||
| key_vertex->GetRhsCandidates() &= key_vertex->GetVertical().GetColumnIndices(); | ||
| key_vertex->SetInvalid(true); | ||
| } | ||
| for (auto key_vertex : key_vertices) { | ||
| key_vertex->GetRhsCandidates() &= key_vertex->GetVertical().GetColumnIndices(); | ||
| key_vertex->SetInvalid(true); |
There was a problem hiding this comment.
Can you explain why we can remove this additional max_fd_error_ check?
Remove AUCC discovery artifacts, perform additional key pruning.
iliya-b
force-pushed
the
tane-refactoring
branch
from
e319415 to
b2465bd
Compare
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.
TANE algorithm implementation missed key pruning (supposedly because of AUCC discovery, which required much larger search space). Since we no longer perform AUCC discovery in this algorithm, original key pruning procedure is now restored.