Rework offset column of LIST #3198
Conversation
There was a problem hiding this comment.
The main problems is you are still allocating buffer/scanning/committing for main column (chunk) after moving offsets out to a offsetColumn(Chunk). so the pr is quite confusing as it is trying to move offsets out of main column, while not fully done.
I think we need a bit more work on this. let's double see if we really need a separate offsetColumn.
| @@ -107,7 +112,9 @@ void ListColumn::scan(Transaction* transaction, node_group_idx_t nodeGroupIdx, | |||
| columnChunk->setNumValues(0); | |||
| } else { | |||
| auto listColumnChunk = ku_dynamic_cast<ColumnChunk*, ListColumnChunk*>(columnChunk); | |||
| Column::scan(transaction, nodeGroupIdx, columnChunk, startOffset, endOffset); | |||
| Column::scan(transaction, nodeGroupIdx, listColumnChunk, startOffset, endOffset); | |||
There was a problem hiding this comment.
If you've moved the offsets to offsetColumn, do we need to scan the list column itself?
| @@ -346,6 +342,7 @@ void ListColumn::prepareCommitForChunk(Transaction* transaction, node_group_idx_ | |||
| } else { | |||
| // we separate the commit into three parts: offset chunk commit, size column chunk commit, | |||
| // data column chunk | |||
| Column::prepareCommitForChunk(transaction, nodeGroupIdx, dstOffsets, chunk, startSrcOffset); | |||
There was a problem hiding this comment.
Ditto here, what are we committing here, as you've moved offsets to offsetColumn as I understand?
| @@ -28,6 +28,8 @@ void ListDataColumnChunk::resizeBuffer(uint64_t numValues) { | |||
| ListColumnChunk::ListColumnChunk( | |||
| LogicalType dataType, uint64_t capacity, bool enableCompression, bool inMemory) | |||
| : ColumnChunk{std::move(dataType), capacity, enableCompression, true /* hasNullChunk */} { | |||
There was a problem hiding this comment.
I think ListColumnChunk is still allocating buffer for offsets. Can you double check?
|
see #3219. |
see #3093
rework offset column