Sprint3 #3
Sprint3 #3
Conversation
Sprint3 tablescan
Fixing template instantiation
| for (auto col_id = ColumnID{0}; col_id < deref_table->col_count(); ++col_id) { | ||
| // add column definition | ||
| // TODO(team): is there a nicer way to combine add_column_definition and emplace_chunk ? | ||
| result_table->add_column(deref_table->column_name(col_id), deref_table->column_type(col_id)); |
There was a problem hiding this comment.
Maybe you could have used add_column_definition.
Is there a reason why you don't use this method?
There was a problem hiding this comment.
In one of the earlier sprints, we added an is_instantiated flag for each column definition, which is only set to true if we call add_column, but not if we only add_column_definition - thus, we used add_column in this case, but weren't quite happy with that
| template <typename T> | ||
| void TableScan::TableScanImpl<T>::_create_pos_list(bool is_reference) { | ||
| // first we need to know which data type we are targeting | ||
| T search_value = type_cast<T>(_search_value); |
| * which, in turn, can only be ValueColumns or DictionaryColumns again | ||
| */ | ||
| auto value_column = std::dynamic_pointer_cast<ValueColumn<T>>(base_column); | ||
| if (value_column != nullptr) { |
There was a problem hiding this comment.
if(value_column) should be sufficient
| } | ||
|
|
||
| auto dictionary_column = std::dynamic_pointer_cast<DictionaryColumn<T>>(base_column); | ||
| if (dictionary_column != nullptr) { |
| @@ -126,8 +126,10 @@ void Table::_validate_existing_definition(const std::string& name, const std::st | |||
| Assert(!_is_instantiated.at(pos), "A column with the given name was already added!"); | |||
| } | |||
|
|
|||
| void emplace_chunk(Chunk chunk) { | |||
| // Implementation goes here | |||
| void Table::emplace_chunk(Chunk chunk) { | |||
There was a problem hiding this comment.
The expected behaviour should be following:
Adds a chunk to the table. If the first chunk is empty, it is replaced. <-- table.hpp
There was a problem hiding this comment.
Good point - we missed that.
|
|
||
| protected: | ||
| void _create_pos_list(bool is_reference); | ||
| template <typename C> |
There was a problem hiding this comment.
You could add empty lines when you have multiline method definitions, especially when they also have template.
There was a problem hiding this comment.
Also, you should add comments for your methods.
| auto chunk = Chunk{}; | ||
|
|
||
| // Fill the chunk with reference columns to mirror the original relation | ||
| for (auto col_id = ColumnID{0}; col_id < deref_table->col_count(); ++col_id) { |
There was a problem hiding this comment.
Writing column_id wouldn't hurt.
|
|
||
| // This function implements the actual scanning | ||
| template <typename T> | ||
| void TableScan::TableScanImpl<T>::_create_pos_list(bool is_reference) { |
There was a problem hiding this comment.
This method does way more than creating a pos list, so it should be named appropriately. Also, a create method that does not return anything is unexpected.
| * only include values in the result that were also referenced from the previous operation | ||
| * we basically scan the entire table again and only include values that are also included | ||
| * in the input PosList | ||
| * this might not be very efficient, but increases readability for now |
There was a problem hiding this comment.
"not very efficient" is an understatement... ;)
| } | ||
|
|
||
| template <typename T> | ||
| template <typename C> |
There was a problem hiding this comment.
What does C stand for? Why is it needed?
| switch (_scan_type) { | ||
| case ScanType::OpLessThan: | ||
| case ScanType::OpLessThanEquals: | ||
| if (first > search_value) { |
There was a problem hiding this comment.
return first > search_value would save the reader from having to read until the end of the method to find out what happens.
No description provided.