sundapeng commented on PR #10905: URL: https://github.com/apache/paimon/pull/10905#issuecomment-6539966765
Thanks for the PR! The page directory looks like the right direction to me, and from my local tests most of the improvement comes from it. But I have some concerns about the format design, mainly the versioning and the compression part. **1. The v1 layout changes again without bumping the version** This is the third v1 layout within one month (#9831: `<BQII` + whole-index zlib, #10891: `<BQIQ` + fixed pages, and now `<BQIQB` + codec + directory), all with version byte `1`. I tried to read an index written by current master (00387fb249) with this PR, and also the reverse direction. Both fail with `Video metadata range is outside the video payload`, because every field after the header is shifted by one byte. The error message is quite misleading. On the read path `VideoKeyframeIndex.open` just throws. On the copy path both Python (`video_format_writer.py:214`) and Java (`VideoFormatWriter.java:336`) validate the copied index, so compaction / rewrite will fail as well. I know none of these commits is released yet, but people building from master may already have such data. Since we already have a version byte, I think we should just use version 2 here (it costs nothing), and let readers report "unsupported keyframe index version" instead of misparsing. Maybe also reserve a flags or header-length field, so next time we don't need to break the layout again. **2. Most of the gain comes from the directory, not compression** I counted GETs with an instrumented `read_at` (synthetic keyframes: constant frame rate, GOP 2, random packet sizes; 512 random `preceding` + `keyframes[...]` lookups): | | master | PR `none` | PR `zlib` | |---|---|---|---| | 65,536 keyframes, open | 2 GET / 197 KB | 1 GET / 1.57 MB | 1 GET / 0.52 MB | | 65,536 keyframes, 512 lookups | 346 GET / 76.9 MB | 0 | 0 | | 1M keyframes, 512 lookups | 3,592 GET / 961 MB | 487 GET / 47.7 MB | 485 GET / 15.5 MB | Compression reduces the bytes by ~3x, but the GET count is the same. On OSS the latency is mostly decided by the number of requests; with a simple model (15 ms per GET + 100 MB/s, my assumption, not measured) it's about 7.8 s vs 7.4 s. For storage, the index is 24 bytes per keyframe while one keyframe usually covers tens of KB of video, so the saving is very small. Also, once a codec is in the spec it's hard to change, and I'm not sure zlib over row-major int64 triples is a good choice. On the same data (GOP 2): - zlib level 1, row-major (this PR): 3.1x - `DeltaVarintCompressor` per column (we already have it in Java and Python for the `.video` footer): 4.0x - per-column delta + zlib level 1: 7.8x So my suggestion is to split this PR: land the directory with codec 0 first, and discuss compression separately. If we keep compression, maybe define the codec as per-column delta encoding. It would also be great if the benchmark could show the directory and compression separately. **3. Unsupported version / codec cannot be distinguished from corruption** If we add codec 2 later, old readers will fail with `Invalid video keyframe index header or codec`, on both read and compaction. But the index is only an accelerator, and we already have the scan path when there is no index. How about: - unsupported version / codec: treat it as no index, log a warning, and copy it as is during compaction; - checksum or monotonicity failure: still fail. **4. Directory caches can be simpler (minor)** Python keeps three caches: directory blocks (512 records, LRU of 2), descriptors (LRU of 512) and pages. The directory for 1M keyframes is only 16.6 KB and is usually already in the 64 KB prefix. If we read the whole directory at open, the binary search needs no I/O and the first two caches can be removed. I also left a few smaller comments inline (page size wording in the spec, the eager-loading threshold, and the temp file in the Java validator). WDYT? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
