haridsv commented on PR #2493: URL: https://github.com/apache/phoenix/pull/2493#issuecomment-4672131656
Just capturing the below feedback I gave offline as the context for the latest change: Question on the per-row long[] of change timestamps: why enumerate each timestamp rather than send a per-row [min, max] range? For the common CDC time-range query, a page covers a contiguous timestamp band within a partition, so a row's cells in [min, max] are exactly its changes for that page — a range and the enumerated array select the same cells. The pre-image below min is still covered: the filter already includes the first cell below the lowest change and then NEXT_COLs, so "one cell past min" needs no extra machinery, and the builder tolerates any surplus in-band versions anyway. A range is also cheaper to build (no per-row TreeSet/array) and smaller to serialize. Is there a case where enumeration prunes meaningfully more than a range would? Want to make sure I'm not missing something the per-timestamp encoding is specifically handling. I believe range covers splits/merges correctly too. A range never under-fetches: every change is in [min, max], and every pre-image is the first cell below its change, which the scan still reaches (the lowest change's pre-image is just the "one cell below min"). A PARTITION_ID is the (hashed) encoded region name, so when a row's region splits/merges, its later changes land under a different PARTITION_ID that can sort before or after the old one — i.e. a single row's change blocks aren't necessarily in time order in the index. A page straddling two such blocks may therefore see a wider [min, max] and pull in a few extra in-band versions — but those are exactly the ones CDCChangeBuilder already discards, so it's bounded over-fetch, not a correctness gap. -- 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]
