Repository navigation
Add the plain codec format to the columnar - #3179
Conversation
064287d to
25f945b
Compare
ee4a315 to
c1ad2da
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1ad2dad3b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if block_index.value_start(0) != 0 || block_index.byte_start(0) != 0 { | ||
| return Err(invalid_data("plain block index does not start at 0")); | ||
| } | ||
| Ok(block_index) |
There was a problem hiding this comment.
When a damaged column contains repeated or decreasing entries in VALUE_STARTS or BYTE_STARTS, open accepts it because only the initial zero entries are checked. Subsequent calls assume strict ordering: block_containing performs a partition-point search and byte_range constructs a slice range, so reading such a column can select the wrong block or panic rather than report invalid data. Validate every adjacent pair in both arrays before returning the index.
Useful? React with 👍 / 👎.
| @@ -241,21 +271,53 @@ static_dynamic_conversions!(Column<DateTime>, DateTime); | |||
| static_dynamic_conversions!(StrColumn, Str); | |||
There was a problem hiding this comment.
This macro is where the None from my comment on open() comes from. Every variant other than Str turns into None, including PlainStr. Since From can't fail, the fact that the column is plain-encoded is lost here.
If str()/bytes() match on the variant directly, could these two invocations (Str/Bytes) go away? Then no new caller can hit the silent None path by accident.
| use crate::column_index::{SerializableColumnIndex, serialize_column_index}; | ||
|
|
||
| /// A block is closed once it holds this many values. | ||
| pub(crate) const PLAIN_BLOCK_MAX_NUM_VALUES: usize = 100_000; |
There was a problem hiding this comment.
The PR description says blocks hold at most 65,536 values and 1 MiB. The code, LAYOUT.md and the module docs say 100,000 values and 10 MiB. Which one is intended? Reading one value loads its whole block, so this matters for random reads.
| /// Creates a ColumnSerializer. | ||
| /// | ||
| /// Precondition: `codec` is `Dictionary` unless `column_type` is `Str` or `Bytes`. | ||
| pub fn start_serialize_column_with_codec<'a>( |
There was a problem hiding this comment.
Nothing stops the same name and type from being written with both codecs. The two keys don't collide, so both columns end up on disk and lookups return whichever comes first. Can we reject the duplicate here, or add a comment that the writer must use one codec per field?
| keywords = ["search", "information", "retrieval"] | ||
| edition = "2021" | ||
| rust-version = "1.88" | ||
| rust-version = "1.91" |
There was a problem hiding this comment.
Does the MSRV bump need a changelog entry? Earlier bumps don't seem to be in CHANGELOG.md so I don't think it's the case.
Adds StrColumnCodec, the column key code of plain columns, the V3 columnar version, and the plain column storage: OnPair16 blocks of at most 65,536 values and 1 MiB, block index, and reader. The columnar writer does not produce plain columns yet.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46c3fddb28
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let block_index = PlainBlockIndex::open(block_index_bytes)?; | ||
| let column_index = open_column_index(column_index_bytes, format_version)?; |
There was a problem hiding this comment.
When a damaged plain column has a valid block index and column index that describe different numbers of values, open accepts both independently. For example, an optional index can reference a value beyond block_index.num_values(), causing append_first_value to panic, while an empty index paired with nonempty blocks silently makes stored values unreachable. Compare the value count represented by non-full column indexes with the block index before returning the column.
Useful? React with 👍 / 👎.
46c3fdd to
41da571
Compare
41da571 to
b3ea895
Compare
Part 1/4 of the plain codec for
Str/Bytesfast fields.Today, text fast fields are always dictionary encoded. The plain codec stores the values themselves, without term ordinals. It is meant for columns with a high cardinality. The codec is chosen by configuration (PR 3), never detected.
This PR adds the format and the storage. The columnar writer does not produce plain columns yet (PR 2).
StrColumnCodec { Dictionary, Plain }.column_typeas before,0x10 | column_typefor plain columns (0x13bytes,0x14str).V3, written only if the columnar contains a plain column. Columnars without plain columns are byte-identical to today. Older readers fail cleanly at open onV3(without the bump,iter_columnswould panic on the unknown key code).onpair0.2.1), codes bitpacked onceil(log2(num_tokens))bits (rawu16at 16 bits), per-block code offsets. Small or incompressible blocks are stored raw.PlainValueReaderkeeps the last block.DynamicColumn::PlainStr/PlainBytes.open_u64_lenientreturns an error on plain columns (they have nou64representation).onpair.columnar/README.md.PlainColumnSerializeris only used by tests until PR 2: it has a temporaryallow(dead_code).Existing tests modified:
format_version::test_version_serialization: 3 valid versions instead of 2.columnar/src/tests.rs: one new arm in two exhaustiveDynamicColumnmatches.🤖 Generated with Claude Code