feat: packed reader and writer new api #172
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shaoting-huang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: shaoting-huang <[email protected]>
shaoting-huang
force-pushed
the
column_group_api
branch
from
7caddc3 to
b7d3ee5
Compare
Signed-off-by: shaoting-huang <[email protected]>
| int NewPackedReader(const char* path, | ||
| CPaths paths, |
There was a problem hiding this comment.
What are the meanings of those two path arguments?
cpp/src/c/packed_reader_c.cpp
Outdated
| @@ -24,11 +24,13 @@ | |||
| #include <memory> | |||
|
|
|||
| int NewPackedReader(const char* path, | |||
| CPaths paths, | |||
| struct ArrowSchema* schema, | |||
| const int64_t buffer_size, | |||
| CPackedReader* c_packed_reader) { | |||
| try { | |||
| auto truePath = std::string(path); | |||
There was a problem hiding this comment.
Is this path only used to initialize fs?
There was a problem hiding this comment.
moved cgo part to milvus
| PackedRecordBatchWriter(std::shared_ptr<arrow::fs::FileSystem> fs, | ||
| std::vector<std::string>& paths, | ||
| std::shared_ptr<arrow::Schema> schema, | ||
| std::shared_ptr<arrow::fs::FileSystem> fs, | ||
| const std::string& file_path, | ||
| const StorageConfig& storage_config); | ||
| StorageConfig& storage_config, | ||
| std::vector<std::vector<int>>& column_groups, | ||
| size_t buffer_size = DEFAULT_WRITE_BUFFER_SIZE); |
There was a problem hiding this comment.
Why bother passing fs instance if we can create it from storage_config?
Or, if fs contains all needed information from storage_config, we can pass fs only.
There was a problem hiding this comment.
Move arrow fs related config out from storage_config to arrow_fs_config
Signed-off-by: shaoting-huang <[email protected]>
shaoting-huang
force-pushed
the
column_group_api
branch
from
87b05fe to
7ad6fce
Compare
Signed-off-by: shaoting-huang <[email protected]>
Signed-off-by: shaoting-huang <[email protected]>
|
|
||
| void Release() {} | ||
|
|
||
| ArrowFileSystemPtr GetArrowFileSystem() { return afs_; } |
There was a problem hiding this comment.
Why bother creating a ArrowFileSystemSingleton instance, if we have the afs_ instance already?
Additionally, we should assert the afs_ exist in such method.
Signed-off-by: shaoting-huang <[email protected]>
shaoting-huang
force-pushed
the
column_group_api
branch
from
f3ec024 to
0a99e0c
Compare
Signed-off-by: shaoting-huang <[email protected]>
Signed-off-by: shaoting-huang <[email protected]>
|
/lgtm |
main feat for the patch:
related: [feat]: packed reader and writer new api #171