[persist] Structured file format #31080
Conversation
bkirwi
force-pushed
the
structured-on-disk-2
branch
2 times, most recently
from
ef767c6 to
60af8bb
Compare
One risk here is that what parallel workloads finds is usually harder to reproduce than other tests. I'll trigger a one-off run at least to see what would happen if we default all of CI to structured. Edit: Done: #31101 |
|
Aight! Note that feature benchmarks and upgrade tests are very likely to fail; unsure about the rest of them. |
|
That kind of failure is ok. I'm more hoping to catch some wrong result or panic. |
We built this for the streaming iterator, but it works well here too.
In particular, don't decode K/Vs when we have an override set, and make sure every K/V only gets decoded once.
bkirwi
force-pushed
the
structured-on-disk-2
branch
from
43bc2c2 to
e60157d
Compare
|
Found and fixed the performance regression - this should be ready for review. (Nightly run with clean benchmarks. The only failure is in data ingest, which I think is the recent Azure issue?) |
Correct, please ignore. |
| let next = if self.part_cursor < self.timestamps.len() { | ||
| let next_idx = self.part_cursor; | ||
| self.part_cursor += 1; | ||
| let mut t = T::decode(self.timestamps.values()[next_idx].to_le_bytes()); |
There was a problem hiding this comment.
This is probably fine, but I'm a bit surprised to see a .to_le_bytes() here? Feels like a bit of an abstraction leakage
There was a problem hiding this comment.
Added a comment about this... it's an artifact of not always reading data from ColumnarRecords, where this code used to live. I've left a comment that we should re-encapsulate it once we're all in on structured data, if that feels like enough to cover it?
Motivation
Adds a new on-disk file format for Persist - the non-dual-write structured-data-only version.
We'd like this format to be supported in our on-prem releases, so folks aren't stuck with the migration midpoint.
Tips for reviewer
I've intentionally left this off in CI, out of the parallel workload. For our existing cloud envs I think it's most important to verify that the (substantial) refactorings here don't cause any regression for existing flag settings. However, it is enabled in parallel workload so we should be able to spot any bugs. (And empirically those tests are good at finding bugs in this corner of Persist.)
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.