-
Notifications
You must be signed in to change notification settings - Fork 0
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
Sprint1 #1
Conversation
TODO: fix todos, do lint, do format |
Add Github Action
Add Github Action
Add Github Action
table: change vector of shared ptr to unique ptr
# Conflicts: # src/lib/storage/table.cpp # src/lib/storage/table.hpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, very thoughtful implementation. Good job.
Greetings to the allgäu!
Co-authored-by: simonstadlinger <33035544+simonstadlinger@users.noreply.github.com>
void Chunk::add_segment(std::shared_ptr<BaseSegment> segment) { | ||
// Implementation goes here | ||
} | ||
void Chunk::add_segment(std::shared_ptr<BaseSegment> segment) { _columns.emplace_back(segment); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if a segment is added to a chunk that already contains values?
const auto dropped_table_count = _tables.erase(name); | ||
Assert(dropped_table_count == 1, "Table could not be removed because it was not found."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good!
|
||
std::vector<std::string> StorageManager::table_names() const { | ||
throw std::runtime_error("Implement StorageManager::table_names"); | ||
auto table_names = std::vector<std::string>{}; | ||
table_names.reserve(_tables.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
src/lib/storage/table.cpp
Outdated
DebugAssert(_chunks.back()->size() != _target_chunk_size, | ||
"Cannot emplace chunk because current last chunk is not full."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this invariant specified?
src/lib/storage/table.cpp
Outdated
if (find_result_iter == _columns.end()) { | ||
throw std::invalid_argument("Column name does not exists."); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be an assert.
|
||
void Table::_append_column_to_chunks(const std::string& type) { | ||
DebugAssert(row_count() == 0, "Cannot append new columns to already existing chunks."); | ||
for (const auto& chunk : _chunks) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If row_count() == 0
there should be only one chunk.
|
||
// Returns a list of all column names. | ||
const std::vector<std::string>& column_names() const; | ||
const std::vector<std::string> column_names() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The public interface was not supposed to be changed. But we can see that you had to do it due to your self-defined Column
data type.
EXPECT_THROW(t.column_id_by_name("no_column_name"), std::exception); | ||
} | ||
|
||
TEST_F(StorageTableTest, GetChunkSize) { EXPECT_EQ(t.target_chunk_size(), 2u); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, the test's name is suboptimal.
@@ -10,38 +10,47 @@ | |||
namespace opossum { | |||
|
|||
StorageManager& StorageManager::get() { | |||
return *(new StorageManager()); | |||
// A really hacky fix to get the tests to run - replace this with your implementation | |||
static StorageManager _instance; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only use a leading underscore for private/protected members/methods but not for local variables, see CONTRIBUTING.md
.
std::vector<std::unique_ptr<Chunk>> _chunks; | ||
std::vector<Column> _columns; | ||
|
||
// TODO(hig): If we need this more often, consider to move this to BaseSegment or ValueSegment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODOs should be done in a submission.
No description provided.