[core] introducing row-range-keyed DeletionVectors for DataEvolution tables#8344
[core] introducing row-range-keyed DeletionVectors for DataEvolution tables#8344steFaiz wants to merge 6 commits into
Conversation
| return false; | ||
| } | ||
|
|
||
| return currentDv.isDeleted(rowId); |
There was a problem hiding this comment.
This reader only evaluates the first row-range DV whose end has not passed the current row id. If two row-range keyed DVs overlap, for example [0, 10] and [5, 15], rows 5-10 are never checked against the second DV, so deletions recorded only there will be returned. I don't see the writer/manifest combiner rejecting overlapping RowIdRangeKey values; they only deduplicate exact keys. Please either enforce non-overlapping row-range DVs when writing/combining metadata, or make this reader evaluate all DVs that may contain the row.
There was a problem hiding this comment.
Thanks for your remind! I've added the check in GlobalCombiner and added a test
| ? null | ||
| : IndexFileMetaSerializer.dvMetasToRowArrayData(dvMetas.values()), | ||
| : IndexFileMetaSerializer.metasToRowArrayData( | ||
| dvMetas, DeletionFileKey.Type.FILE_NAME), |
There was a problem hiding this comment.
For row-range keyed DVs this renders the compatibility marker row instead of the real row-id ranges, because metasToRowArrayData(..., FILE_NAME) intentionally fast-fails old readers. That makes table_indexes.dv_ranges misleading for data-evolution tables. Can we expose the row-range DV schema here as well, or render the ranges in a separate column, instead of showing the legacy marker?
There was a problem hiding this comment.
This is very helpful! I've unified FileNameKey and RowRangeKey:
- Reusing the same field
- For FileNames, display the file name
- For RowRanges, display the formated range. e.g. [0, 100]
|
I thought about the design and I think that making row ranged-DV aligned with normal file ranges is much more simple and easy to manage, compared to fully decoupled mode. Just like filename-based implementation.
This is because it's complicated to update DVs if we let DVs can be fully decouple, consider this scenario: // Current DV:
[0, 100] , [101, 200], [230, 300]Update a new DV range [50, 250]
If we don't design sophiscated merge/split mechanism, there might be some DVs' size too big or too small. I think we could:
@JingsongLi Could you please review this proposal and share your thoughts? Any feedback would be greatly appreciated! |
|
@steFaiz If we move in this direction, do we maintain another simple design: Set the DV according to the oldest data file, still using the file name, which will not introduce any new mechanism and maintain the old DV mechanism. |
|
@JingsongLi Thanks for your reply! This design: "Reference to the oldest file" is also in my original design docs.
I agree that the oldest filename based approach will reduce code changes. Which one do the community prefer? range based or the oldest filename based? I'll modify this PR! |
|
@steFaiz Thank you very much for your reply, it is very valuable. Indeed, both options will bring different levels of complexity, and we need to choose between them. I am also very conflicted because the complexity they bring is almost of the same magnitude. I prefer the older filename based approach because it preserves the original DV mechanism and eliminates the need to worry about maintaining the row id ranges. We can consider retaining the original file information in DV mode until the DV is generated. |
Purpose
The first part of #8322
This is a very BIG pr, I tried to split it into several sub PRs, but found it very hard.
The PR only focuses on core module, leaving Spark/Flink module untouched, mainly includes:
DeletionFileKeyto replace currentfilenameas the unique identifier of each DeletionFileDataEvolutionApplyDvReaderto filter out deleted row ids. This class refers toIndexedSplitRecordReader: First enrich readType by_ROW_ID, then filter rows by RowRange DeletionVectors.SnapshotReaderImpl, assigning overlapping row range DeletionVectors to each DataSplit.Besides, most changes are about changing many
Stringfields toDeletionFileKey.Tests
Unit Tests only.