Arrow rel copier #1154
Arrow rel copier #1154
Conversation
weipang142857
requested review from
ray6080,
semihsalihoglu-uw and
printfCalvin
src/include/storage/in_mem_csv_copier/in_mem_arrow_rel_copier.h
Outdated
Show resolved
Hide resolved
weipang142857
force-pushed
the
arrow_rel
branch
from
0225694
to
b2ca065
Compare
| const string filePath; | ||
| const CSVReaderConfig csvReaderConfig; | ||
| shared_ptr<ReaderConfig> readerConfig; |
There was a problem hiding this comment.
Why is this a shared_ptr? I think it should be a unique_ptr and you can share the raw pointer if you have to. I see you say this:
Using unique_ptr leads to some errors, which indicates that it relates to calling the destructor of CopyDescription, I think it's because of using std::move on CopyDescription in the constructor of some other class when initializing. Using shared_ptr avoids the error.
Whatever it is, we need to first understand why unique_ptr does not work. It could be a bug. We can't fix a problem without understanding it. So whatever the problem, first let's understand it and find a fix (I don't think there is any good reason for this to be shared_ptr.)
There was a problem hiding this comment.
This does not seem to be a bug. There are a few places in the code base that copies the CopyDescription object, which leads to copying the unique_ptr. Since unique_ptr does not allow having multiple pointers pointing to the same object, it does not have a copy constructor. Therefore, an error about calling an implicitly-deleted copy constructor is triggered. Using shared_ptr avoids this issue.
| void InMemArrowRelCopier::putPropsOfLineIntoLists(InMemArrowRelCopier* copier, | ||
| vector<PageByteCursor>& inMemOverflowFileCursors, const vector<shared_ptr<T>>& batchColumns, | ||
| const vector<nodeID_t>& nodeIDs, const vector<uint64_t>& reversePos, int64_t blockOffset, | ||
| int64_t& colIndex, char delimiter) { |
There was a problem hiding this comment.
Let me clarify what I was referring to when saying it was a hack. In initCSVReader there is this line:
arrowParse.delimiter = copyDescription.readerConfig->tokenSeparator;
Why is delimter being set to tokenSeparator? This looks like a hack. tokenSeparator is a CSV-specific option we are giving. It has nothing to do with the List data type. So again, let's discuss this.
Update: I think I now understand why I was confused. The reason is ReaderConfig::tokenSeparator is a confusing name because it should be called ReaderConfig::delimiter. I left a comment on this in csv_reader.h.
weipang142857
force-pushed
the
arrow_rel
branch
from
e535966
to
ef12216
Compare
| CopyDescription::CopyDescription(const CopyDescription& copyDescription) | ||
| : filePath{copyDescription.filePath}, readerConfig{nullptr}, fileType{ | ||
| copyDescription.fileType} { | ||
| if (fileType == FileType::CSV) { |
There was a problem hiding this comment.
Check for this null or not condition directly on the readerConfig field. So something like:
if (copyDescription.readerConfig) {
this->readerConfig = make_unique<CSVReaderConfig>(*copyDescription.readerConfig);
}
20f39f1
to
7167f06
Compare
ray6080
added
the
feature
Implemented arrow rel copier
Refactored code of both arrow rel copier and arrow node copier
Deleted old rel copier
Changed some test files to accommodate CSV standard