deepakpanda93 commented on code in PR #19730:
URL: https://github.com/apache/hudi/pull/19730#discussion_r3853109634
##########
website/learn/tech-specs.md:
##########
@@ -295,11 +295,38 @@ This denotes that the previous action that wrote the log
block was unsuccessful.
### Delete Block (Id: 1)
+A delete block carries the tombstones for records deleted by a commit. Within
a batch it is always written after the
+data (Avro/HFile/Parquet) block, so that the deletes are applied after the
inserts and updates they accompany.
+
| Section | #bytes | Description |
| ---| ---| --- |
-| format version | 4 | version of the log file format |
-| length | 8 | length of the deleted keys section to follow |
-| deleted keys | variable | Tombstone of the record to encode a delete. The
following 3 fields are serialized using the KryoSerializer. **Record Key** -
Unique record key within the partition to deleted **Partition Path** -
Partition path of the record deleted **Ordering Value** - In a particular batch
of updates, the delete block is always written after the data
(Avro/HFile/Parquet) block. This field would preserve the ordering of deletes
and inserts within the same batch. |
+| block version | 4 | The log block version the writer emitted. The reader
selects the payload encoding below from this value. |
+| length | 4 | Length in bytes of the payload to follow |
+| payload | variable | The serialized tombstones, in the encoding selected by
the block version |
+
+The payload encoding has changed twice, and readers dispatch on the block
version so blocks written by older writers
+remain readable:
+
+| Block version | Encoding | Tombstone contents |
+|---------------|----------|--------------------|
+| 1 | Kryo-serialized `HoodieKey[]` | Record key and partition
path only. These blocks carry no ordering value. |
+| 2 | Kryo-serialized `DeleteRecord[]` | Record key, partition
path and ordering value. |
+| 3 | Avro, binary-encoded `HoodieDeleteRecordList` | Record key,
partition path and a typed ordering value. |
+
+Version 3 is what current writers produce. Encoding the payload with Avro
rather than Kryo makes a delete block readable
+by any Avro implementation instead of only a JVM with matching Kryo
registrations, and it gives the ordering value a
+declared type rather than leaving it an opaque serialized object.
+
+Each element of the version 3 `deleteRecordList` array is a
`HoodieDeleteRecord`:
+
+| Field | Type | Description |
+|-------|------|-------------|
+| `recordKey` | nullable `string` | Unique record key within the partition
being deleted. |
+| `partitionPath` | nullable `string` | Partition path of the record being
deleted. |
+| `orderingVal` | nullable union of typed wrappers | Ordering value used to
resolve merge order against other writes to the same key, encoded with the
wrapper matching the value's own type: `BooleanWrapper`, `IntWrapper`,
`LongWrapper`, `FloatWrapper`, `DoubleWrapper`, `BytesWrapper`,
`StringWrapper`, `DateWrapper`, `DecimalWrapper`, `TimeMicrosWrapper`,
`TimestampMicrosWrapper` or `ArrayWrapper`. |
+
+A delete block may also carry the [`RECORD_POSITIONS`](#headers) header. When
it does, the reader can apply the deletes
+positionally against the base file named by
`BASE_FILE_INSTANT_TIME_OF_RECORD_POSITIONS` instead of resolving each key.
Review Comment:
Good catch, that reads wrong. Fixed in 62a96e91.
You are right that the header holds an instant time. `HoodieAppendHandle`
writes `baseInstantTimeForPositions` into it as a `String`:
```java
if (baseInstantTimeForPositions.isPresent()) {
updatedHeader.put(
HeaderMetadataType.BASE_FILE_INSTANT_TIME_OF_RECORD_POSITIONS,
baseInstantTimeForPositions.get());
}
```
and the reader gets it back through
`HoodieLogBlock#getBaseFileInstantTimeOfPositions`, whose javadoc is explicit
that it returns the "base file instant time of the record positions". Nothing
in the block carries a path, so "named by" was misleading about what the reader
actually has to work with.
What tipped it from a wording nit to something worth fixing is that my
sentence was also inconsistent with the rest of the same page. The Headers
table already describes this header as the *"Begin (requested) instant time of
the base file that the `RECORD_POSITIONS` bitmap is relative to"*, and the
log-merge section already says *"the base file identified by
`BASE_FILE_INSTANT_TIME_OF_RECORD_POSITIONS`"*. I had introduced a third
phrasing for the same thing, in a spec where consistency is most of the value.
It now reads:
> A delete block may also carry the `RECORD_POSITIONS` header. When it does,
the reader can apply the deletes positionally instead of resolving each key,
against the base file identified by the instant time in
`BASE_FILE_INSTANT_TIME_OF_RECORD_POSITIONS`. The reader matches that instant
time against the base file of the file slice, which is also how it validates
that the positions still refer to the base file the bitmap was built against.
I added that last clause because it answers the question your comment
implicitly raises: why carry an instant time rather than something more direct.
The Headers table already gives the reason ("Used by the reader to validate
that positions still refer to the same base file"), so this connects the delete
block note to it instead of leaving the reader to infer the purpose.
Build passes with the warning block still byte-identical to a baseline built
at the same base commit, and the rendered page no longer contains "named by".
Thanks also for verifying the byte widths, the version dispatch, the schema
field names, the full wrapper list and the current block version against
`hudi-common`. Agreed on routing to a committer. @yihua, the two things I would
most value a committer's eye on are the v1 claim that those blocks carry no
ordering value at all, and whether the "block version" relabel is the
terminology the project wants in the spec, given the log *file* format version
is separately documented as 1 on the same page.
--
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]
