Bitpacking mode info #15623
Bitpacking mode info #15623
Conversation
…Santa did not bring it;
so I tried myself ;-)
This pull request will do:
- Create a BitpackingSegmentState that overrides GetSegmentInfo to return mode info
- Add BitpackingInitSegment that returns segment-info (used in ColumnData)
- Add BitpackingInitSegment / init_segment to the compressionfunction
(I also renamed BitpackingCompressState to BitpackingCompressionState for
consistency with other compression code.)
The approach implemented seems to work correctly and passes tests locally.
However, the new information is only shown when you first close the database and reopen it.
If you only use `CHECKPOINT`, `init_segment` is not called and the modes not reported.
I am stuck where to fix this; it seems that this would be a step related to `ConvertToPersistent`
that I overlook but I do not understand what I need to do to make it work.
I have tested using the following SQL (but because of this incomplete behaviour not yet added as a test):
```
pragma force_compression = 'BitPacking';
CREATE or replace TABLE test (a INTEGER, b INTEGER);
insert into test values (10,12), (11,12), (12,11), (NULL,NULL);
insert into test values (10,12), (33,33), (33,33), (10,12);
select segment_id, row_group_id, block_id, block_offset, compression, segment_info FROM pragma_storage_info('test') order by segment_id, row_group_id asc;
checkpoint;
select segment_id, row_group_id, block_id, block_offset, compression, segment_info FROM pragma_storage_info('test') order by segment_id, row_group_id asc;
```
After closing the CLI and restarting it on the same database, and then doing
```
select segment_id, row_group_id, block_id, block_offset, compression, segment_info FROM pragma_storage_info('test') order by segment_id, row_group_id asc;
```
the `pragma_storage_info` reports `segment_info` that includes the bitpacking mode.
Hoping for some DuckDB compression guru advice, A.
```
D select segment_id, row_group_id, block_id, block_offset, compression, segment_info FROM pragma_storage_info('test') order by segment_id, row_group_id asc;
┌────────────┬──────────────┬──────────┬──────────────┬──────────────┬──────────────┐
│ segment_id │ row_group_id │ block_id │ block_offset │ compression │ segment_info │
│ int64 │ int64 │ int64 │ int64 │ varchar │ varchar │
├────────────┼──────────────┼──────────┼──────────────┼──────────────┼──────────────┤
│ 0 │ 0 │ 1 │ 0 │ BitPacking │ Mode: for │
│ 0 │ 0 │ 1 │ 48 │ Uncompressed │ │
│ 0 │ 0 │ 1 │ 304 │ BitPacking │ Mode: for │
│ 0 │ 0 │ 1 │ 352 │ Uncompressed │ │
└────────────┴──────────────┴──────────┴──────────────┴──────────────┴──────────────┘
```
…to bitpacking-mode-info
…er processing, simpler tests).
| return std::move(result); | ||
| } | ||
|
|
||
| void CleanupState(ColumnSegment &segment) { |
There was a problem hiding this comment.
I don't think we need this, cleanup state is necessary for state that holds lifetimes outside of the scope of the segment, such as additional blocks that it has created - which is what the UncompressedStringSegmentState uses it for.
UncompressedStringStorage can create additional blocks to hold "overflow" strings, which are bigger than a certain threshold (I think it's 4096 by default)
When the column segment that these additional blocks are a part of gets dropped, we need to inform the block manager that these additional blocks can now also be safely reused.
There was a problem hiding this comment.
Of course, should have realized. Will remove.
There was a problem hiding this comment.
Just out of curiosity - we now write extra information, could it happen that this requires an extra block (in the case where everything was filled completely with data and I want to write out the bitmode histogram) or does this extra serialization data get written to a separate place in storage?
There was a problem hiding this comment.
Thanks, this looks like it'll work from reading the code 👍
I do however want to see at least one test making sure this is working properly
Looking at:
static constexpr const idx_t BITPACKING_METADATA_GROUP_SIZE = STANDARD_VECTOR_SIZE > 512 ? STANDARD_VECTOR_SIZE : 2048;We should be deciding per 2048 values which mode to use, so given 10000 tuples to compress, this should result in a total of 5 groups
(also your test should probably use require vector_size 2048 because the vector size will impact the group size)
…oved use of class template as T is not needed.
There was a problem hiding this comment.
Thanks for the PR!
Unless I'm misunderstanding the PR - this seems to store more information in the database only to provide the segment information during the storage_info callback. The new serialize/deserialize logic will also hurt forwards compatibility of database files.
Given this information exists elsewhere already in the blocks, perhaps we can instead introduce a separate get_segment_info callback to the compression method so that we don't need to introduce the new compressed segment state?
|
The PR adds query I
SELECT
segment_info
FROM
pragma_storage_info('test_bp')
WHERE segment_type NOT IN ('VALIDITY')
----
CONSTANT: 9, DELTA_FOR: 1I'm not sure about the serialization / forwards compatibility complications, but this sounds like a welcome change for observability ? I also feel like Logging this information during checkpointing would be a nicer way to provide this observability, waiting patiently on #15119 🙏 I think I just understood this part:
This is the existing code to get segment info auto segment_state = segment->GetSegmentState();
if (segment_state) {
column_info.segment_info = segment_state->GetSegmentInfo();
column_info.additional_blocks = segment_state->GetAdditionalBlocks();
}We can add a |
|
The change itself is welcome for sure - my point is that this information is already stored in the (compressed) data itself. We should be able to just read it from the blocks. That then (1) also works with older database files, (2) does not require storing additional data in the database file, increasing the size, and (3) does not break the ability of older DuckDB versions from reading this compression due to the introduction of new information they do not understand. |
|
Thanks Mark! My initial approach used only init-segment, but didn't work at the moment of checkpointing; in discussion we switched to this design. I guess I can alternatively scan all segments in init-segment to recreate this information upon the column being loaded from disk. Have to look into the exact mechanics but will try. (It may imply a longer load time?) |
void LoadNextGroup() {
D_ASSERT(bitpacking_metadata_ptr > handle.Ptr() &&
bitpacking_metadata_ptr < handle.Ptr() + current_segment.GetBlockManager().GetBlockSize());
current_group_offset = 0;
current_group = DecodeMeta(reinterpret_cast<bitpacking_metadata_encoded_t *>(bitpacking_metadata_ptr));
bitpacking_metadata_ptr -= sizeof(bitpacking_metadata_encoded_t);All the group metadata is neatly packed together, so scanning just that portion should be cheap and efficient The map<BitpackingMode, idx_t> counts;
auto tuple_count = segment.count.load();
BitpackingScanState<T> scan_state(segment);
for (idx_t i = 0; i < tuple_count; i += BITPACKING_METADATA_GROUP_SIZE) {
counts[scan_state.current_group.mode]++;
}
// stringify the counts and returnshould be all you need 👍 |
|
Yes, so you were mostly on the right track to do what Mark is suggesting, but the place it was being done was wrong. See the second part of #15623 (comment) |
…to bitpacking-mode-info
|
So followed the suggestion to use the map but kept segment_info a column of VARCHAR for now. LMK if you'd rather see that changed. Otherwise seems good to go. |
|
(only removed two includes, it should pass tests as before) |
|
Looks like they were not unnecessary :) |
Turn return type into a InsertionOrderPreservingMap<string> similar to PhysicalOperator::ParamsToString (following suggestion by Mark). Keep segment_info column of type VARCHAR to limit changes.
3ad9d04 to
b464979
Compare
|
OK don't believe the LSP without checking twice... |
|
Will not touch it any more! 😀 |
|
Thanks! |
Bitpacking mode info (duckdb/duckdb#15623)
Bitpacking mode info (duckdb/duckdb#15623)
Bitpacking mode info (duckdb/duckdb#15623)
Bitpacking mode info (duckdb/duckdb#15623)
Bitpacking mode info (duckdb/duckdb#15623)
Bitpacking mode info (duckdb/duckdb#15623)
Version of #15622 without (I think) merge conflicts, which was a continuation of #15544
This implements the suggested serialize/deserialize, in line with the suggestions by @Tishj (and building on his initials steps).
More detailed info in #15544