rangareddy commented on PR #9056: URL: https://github.com/apache/hudi/pull/9056#issuecomment-5263409703
Apologies @parisni — I opened #19590 for #16063 (HUDI-6456) without spotting this PR first. My mistake: I searched the issue timeline for cross-referenced PRs, and because this predates the JIRA-migrated issue it never linked, even though hudi-bot posted a "Linked PR(s)" comment naming it. This PR is the original and yours should be the one that lands; I have said so on mine. Since I had already read the code closely, here is what I verified against current master (`3ba31dd37fff`), offered as review rather than a competing change. Happy to push these to your branch if you would rather not. **1. Worth adding — the config name collision.** `hoodie.parquet.bloom.filter.enabled` is a *different feature* from the `parquet.bloom.filter.enabled#<col>` keys this page documents, and the names differ only by the `hoodie.` prefix. The Hudi one controls whether a bloom of **record keys** goes into the footer for the bloom index during upserts: `HoodieStorageConfig:252-258` (default `true`, since 0.15.0) and `HoodieFileWriterFactory.enableBloomFilter:141-146` (`populateMetaFields && (thisConfig || index.type contains BLOOM)`). A reader who finds only that row in `configurations.md` could reasonably conclude it enables what this page describes. One admonition would close that off. **2. Worth adding — the settings fail silently.** `HoodieBaseParquetWriter.handleParquetBloomFilters:98-119` resolves `withBloomFilterEnabled` / `withBloomFilterNDV` reflectively and swallows `NoSuchMethodException` (`:107-109`, `:116-118`). On a Parquet that predates those builder methods the keys are ignored with no error, which is a confusing thing to debug without a note. **3. The COW-only note looks too strong.** Reading the write paths, MOR **base** files are written by the same `HoodieAvroFileWriterFactory` → `HoodieAvroParquetWriter extends HoodieBaseParquetWriter`, so they go through `handleParquetBloomFilters` exactly as COW base files do. What MOR does not get is blooms on its **log blocks**: `HoodieParquetDataBlock.serializeRecords:91-99` takes a separate `serializeRecordsToLogBlock` path with its own small params map (compression, ratio, dictionary) and never sees the Hadoop bloom keys. So the accurate limitation reads more like "base files only — MOR log blocks do not carry them", rather than "COW tables only". I have not run this, so please sanity-check me. **4. Two version nits.** "starting from hudi 1.14.0" should be 0.14.0. And "Since Hudi 0.14.0" is right — I checked the tags, `handleParquetBloomFilters` is absent in `release-0.13.1` and present from `release-0.14.0` on. **5. Versioned docs.** This adds `website/docs/` only. Since the feature has shipped since 0.14.0, every versioned copy on the site is missing it. Recent docs PRs that apply across supported releases fan out to `version-1.0.0`, `1.0.1`, `1.0.2`, `1.1.1` and `1.2.0` (see #19555 and #19459), so this probably wants the same five. Also, the new page is not added to `website/sidebars.js` as far as I can see — worth checking it is reachable other than through the `configurations.md` link. -- 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]
