Refactor to avoid confusing "raw block" #10408
Conversation
Summary: We have a lot of confusing code because of mixed, sometimes completely opposite uses of of the term "raw block" or "raw contents", sometimes within the same file. For example, in BlockBasedTableBuilder, raw_block_contents and raw_size generally referred to uncompressed block contents and size, while WriteRawBlock referred to writing a block that is already compressed if it is going to be. Meanwhile, in BlockBasedTable, raw_block_contents either referred to a (maybe compressed) block with trailer, or a maybe compressed block maybe without trailer. (Note: left as follow-up work to use C++ typing to better sort out the various kinds of BlockContents.) This change primarily tries to apply some consistent terminology around the kinds of block representations, avoiding the unclear "raw". (Any meaning of "raw" assumes some bias toward the storage layer or toward the logical data layer.) Preferred terminology: * **Serialized block** - bytes that go into storage. For block-based table (usually the case) this includes the block trailer. * **Maybe compressed block** - like a serialized block, but without the trailer (or no promise of including a trailer). Must be accompanied by a CompressionType. * **Uncompressed block** - "payload" bytes that are either stored with no compression, used as input to compression function, or result of decompression function. * **Parsed block** - an in-memory form of a block in block cache, as it is used by the table reader. Different C++ types are used depending on the block type (see block_like_traits.h). Other refactorings: * Misc corrections/improvements of internal API comments * Remove a few misleading / unhelpful / redundant comments. * Use move semantics in some places to simplify contracts * Use better parameter names to indicate which parameters are used for outputs * Remove extraneous `extern` * Various clean-ups to CacheDumperImpl (mostly unnecessary code) Test Plan: existing tests
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@pdillinger has updated the pull request. You must reimport the pull request before landing. |
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
utilities/cache_dump_load_impl.cc
Outdated
| // create the raw_block_content based on the information in the dump_unit | ||
| BlockContents raw_block_contents( | ||
| Slice((char*)dump_unit.value, dump_unit.value_len)); | ||
| // create the serialized_block based on the information in the dump_unit |
There was a problem hiding this comment.
Is serialized_block also uncompressed block?
There was a problem hiding this comment.
I think you're right. These do not include block trailer so should be called "uncompressed block" not "serialized block". I'll fix that. 👍
|
@pdillinger has updated the pull request. You must reimport the pull request before landing. |
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@pdillinger has updated the pull request. You must reimport the pull request before landing. |
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: We have a lot of confusing code because of mixed, sometimes
completely opposite uses of of the term "raw block" or "raw contents",
sometimes within the same source file. For example, in
BlockBasedTableBuilder,raw_block_contentsandraw_sizegenerally referred to uncompressed blockcontents and size, while
WriteRawBlockreferred to writing a block thatis already compressed if it is going to be. Meanwhile, in
BlockBasedTable,raw_block_contentseither referred to a (maybecompressed) block with trailer, or a maybe compressed block maybe
without trailer. (Note: left as follow-up work to use C++ typing to
better sort out the various kinds of BlockContents.)
This change primarily tries to apply some consistent terminology around
the kinds of block representations, avoiding the unclear "raw". (Any
meaning of "raw" assumes some bias toward the storage layer or toward
the logical data layer.) Preferred terminology:
(usually the case) this includes the block trailer. WART: block
sizemay ormay not include the trailer; need to be clear about whether it does or not.
trailer (or no promise of including a trailer). Must be accompanied by a
CompressionType.
compression, used as input to compression function, or result of
decompression function.
used by the table reader. Different C++ types are used depending on the
block type (see block_like_traits.h).
Other refactorings:
outputs
externCacheDumperImpl(mostly unnecessary code)Test Plan: existing tests