rangareddy opened a new pull request, #19586:
URL: https://github.com/apache/hudi/pull/19586
### Describe the issue this Pull Request addresses
Closes #16697 (HUDI-8421): *"Right now the user must provide
`getMandatoryFieldsForMerging` in record merger
implementation to make sure the functionality is correct. We need to inform
the developer through our docs."*
`record_merger.md` is where a developer lands to implement a custom merger.
It sketches the interface as
`merge`, `partialMerge`, `getRecordType`, `getMergingStrategy`, and follows
with five Implementation
Guidelines. Neither the sketch nor the guidelines mentions
`isProjectionCompatible()` or
`getMandatoryFieldsForMerging()` — the pair that decides whether a
Merge-on-Read read may project columns
and, if it may, which columns the merger still needs. So the two methods
that determine whether a custom
merger produces correct results on a projected read were invisible in the
one place you would look for them.
What the code does, and what the docs now say:
- `isProjectionCompatible()` defaults to **`false`**, and for a `CUSTOM`
merger
`FileGroupReaderSchemaHandler` then returns `tableSchema` — the
full-schema read. Always correct, never
pruned.
- Once it returns `true`, the reader reads the query's columns **plus**
those named by
`getMandatoryFieldsForMerging()`; for `CUSTOM` mode that method is the
only source of the extra columns.
- Its default is the record key field plus the ordering fields, so a merger
that reads anything else must
declare it.
### Summary and Changelog
- Added `isProjectionCompatible()` and `getMandatoryFieldsForMerging()` to
the interface sketch, each with a
comment on its default.
- Added a sixth Implementation Guideline, and a **Projection compatibility**
subsection covering the pairing,
the performance trade-off, and the failure mode.
- The worked example mirrors `MaxRankRecordMerger` in `hudi-trino`'s tests —
real compiling code in this repo
— rather than an invented snippet: it preserves the interface defaults
through
`HoodieRecordMerger.super.getMandatoryFieldsForMerging(...)` and adds its
own column.
- Corrected the `partialMerge` signature in the sketch, which still showed
Avro `Schema` for the reader
schema. `release-1.2.md` already lists `partialMerge` among the
source-breaking signature changes.
### Verification
This is a docs change, so there is no test to add — stated plainly rather
than implied. Every claim was read
off master instead of recalled:
| claim in the docs | verified against |
| --- | --- |
| `isProjectionCompatible()` defaults to `false` |
`HoodieRecordMerger.java:139-141` |
| default `false` ⇒ full table schema for `CUSTOM` |
`FileGroupReaderSchemaHandler.java:215-218` (returns `this.tableSchema`) |
| mandatory fields only consulted once projection compatible | same early
return, plus `:245-247` where `CUSTOM` delegates to the merger |
| reader reads requested **+** mandatory fields |
`FileGroupReaderSchemaHandler.java:221-234` |
| default = record key + ordering fields | `HoodieRecordMerger.java:146-161`
|
| commit-time and event-time ordering are projection compatible |
`HoodieRecordMerger.java:137` |
| the `super`-preserving example compiles |
`MaxRankRecordMerger.java:73-83`, the only in-tree caller of that default |
**Three files, and they are deliberately not identical.** The signature
differs by release, checked at the
release refs rather than assumed:
- `website/docs/` and `versioned_docs/version-1.2.0/` — `HoodieSchema`
(`release-1.2.0` line 146)
- `versioned_docs/version-1.1.1/` — Avro `Schema` (`release-1.1.1` line
147), so `partialMerge` there is
already correct and is left untouched; only the two new methods are added,
with the 1.1 type
`versioned_docs/version-1.0.x` carries the interface sketch but has no
Implementation Guidelines section, so
it is out of scope.
Markdown checked by running `markdownlint` over the three files **before and
after**, and comparing rule
classes rather than raw counts, since the pristine files already have 237
`MD013` findings:
```
before: MD013/line-length MD033/no-inline-html
after : MD013/line-length MD033/no-inline-html -> no new class of finding
```
That comparison caught two things I had introduced and have fixed: `-`
bullets where this file uses `*`
(`MD004`), and a double blank line (`MD012`). Code-fence count is even in
all three files, and the RFC-101
pointer stays at the end of Implementation Guidelines rather than drifting
under the new subsection.
**Not done:** I did not run the Docusaurus build (`website/node_modules` is
absent and a full install is
heavy). The change adds only fenced code blocks and prose using constructs
already present in the same file,
and introduces no inline HTML, so MDX risk is low — but I would rather say
so than imply a build passed.
### Impact
Documentation only. No code, config, API or format change. A developer
implementing a custom `HoodieRecordMerger`
can now find out from the docs that projecting reads and declaring merge
columns are coupled, instead of from a
wrong query result.
### Risk Level
none
### Documentation Update
This *is* the documentation update. Targets the `asf-site` branch; no
release note needed.
### Contributor's checklist
- [x] Read through [contributor's
guide](https://hudi.apache.org/contribute/how-to-contribute)
- [x] Enough context is provided in the sections above
- [x] Adequate tests were added if applicable — n/a for a docs change;
verification table above instead
- [ ] CI passes on my PR — `asf-site` PRs do not run the `master` gates;
`markdownlint` parity checked
locally as described
--
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]
