Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DRAFT] DynFD algorithm (2/4 search strategies) #462

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Vdaleke
Copy link
Collaborator

@Vdaleke Vdaleke commented Sep 15, 2024

Implemented part of DynFD algorithm with 2 base strategies and initialization with HyFD. Added tests for DynFD. Added dynamic relation data, dynamic position list index and NonFD prefix tree. FDTree and FDTreeVertex classes moved to model and reused with several new methods.

@Vdaleke Vdaleke changed the title [DRAFT] DynFD algorithm [DRAFT] DynFD algorithm (2/4 search strategies) Sep 15, 2024
Copy link
Contributor

@rpuwa rpuwa left a comment

Choose a reason for hiding this comment

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

пока что всё, больше придирался к кодстайлу, но пару глобальных моментов надо обговорить всё же

src/core/model/table/vertical_map.cpp Outdated Show resolved Hide resolved
#include <fd/fd.h>
#include <fd/fd_algorithm.h>
#include <tabular_data/input_table_type.h>

Copy link
Contributor

Choose a reason for hiding this comment

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

все инклюды на строчках 2-6 по идее нужно прописывать вот так
#include "path/to/header"

а ещё algorithm.h, fd.h, d/hycommon/types.h тут не нужны, зато нужно точно прописать #include <unorderd_set> и <memory>, потому, что их надо явно прописывать там, где с ними работаешь

break;
}

LOG(TRACE) << "Cycle done";
Copy link
Contributor

Choose a reason for hiding this comment

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

мб это хочется до прерывания цикла писать?


comparison_suggestions = validator.ValidateAndExtendCandidates();

if (comparison_suggestions.empty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do { ... } while(...) цикл выглядит опрятнее


bool const is_non_fd_validation_needed =
(!delete_statement_indices_.empty()) || (update_statements_table_ != nullptr);
bool const is_fd_validation_needed =
Copy link
Contributor

Choose a reason for hiding this comment

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

по хорошему переместить на 80 строчку

#include <memory>
#include <vector>

#include <boost/dynamic_bitset.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

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

про хедеры всё же дополню на конкретном примере:
4) не нужно писать хедер в filename.cpp, если он прописан в filename.h

size_t rhs, size_t cur_bit,
std::vector<boost::dynamic_bitset<>>& result) const;

void GetSpecialsRecursive(boost::dynamic_bitset<> const& lhs, boost::dynamic_bitset<>& cur_lhs,
Copy link
Contributor

Choose a reason for hiding this comment

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

хочется, чтобы определение Generals/Specials было отражено в комментариях, с первого взгляда непонятно о чём речь

size_t sortedPlisIndex,
std::vector<std::shared_ptr<DPLI>>& sorted_plis,
size_t const rhs) {
sortedPlisIndex++;
Copy link
Contributor

Choose a reason for hiding this comment

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

придирка ради придирки, но ++ можно под иф занести префиксным инкрементом

}
}
for (auto const& next_cluster : intersection) {
return FindClusterViolating(next_cluster, sortedPlisIndex, sorted_plis, rhs);
Copy link
Contributor

Choose a reason for hiding this comment

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

return в цикле foreach - что-то сомнительное

Comment on lines +77 to +78
bit = lhs.find_next(bit)) {
sorted_plis.push_back(relation_->GetColumnData(bit).GetPositionListIndex());
Copy link
Contributor

Choose a reason for hiding this comment

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

стоит прогнаться по всем файлам с clang-format, чтобы все такие моменты подправить

@Vdaleke Vdaleke force-pushed the feature/dynfd-algorithm branch 3 times, most recently from 3dc088d to 02accc0 Compare October 7, 2024 21:20
@Vdaleke Vdaleke force-pushed the feature/dynfd-algorithm branch 3 times, most recently from 88f4994 to 3e25c55 Compare November 17, 2024 21:28
@Vdaleke Vdaleke force-pushed the feature/dynfd-algorithm branch from 3e25c55 to 06db3a9 Compare November 18, 2024 10:16
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.

2 participants