Skip to content

Support _etag virtual column for HDFS storage - #108255

Merged
tiandiwonder merged 8 commits into
ClickHouse:masterfrom
zhangyifan27:hdfs-etag-support
Jul 27, 2026
Merged

Support _etag virtual column for HDFS storage#108255
tiandiwonder merged 8 commits into
ClickHouse:masterfrom
zhangyifan27:hdfs-etag-support

Conversation

@zhangyifan27

@zhangyifan27 zhangyifan27 commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

HDFS has no native ETag, so synthesise a version identifier from the fields exposed by hdfsFileInfo: the modification time and the size. Populate it in both tryGetObjectMetadata and listObjects so the _etag virtual column is vailable on HDFS (previously it was always empty).

The token has only second precision: libhdfs3 truncates the NameNode millisecond mtime to seconds in mLastMod, so a rewrite within the same wall-clock second that keeps the exact same size collides. Because it is not a strong content identifier, it is marked with ObjectMetadata::etag_is_strong = false and is deliberately NOT used as a content-cache key: the Parquet metadata cache, the filesystem cache and the page cache still skip HDFS (they require a strong etag), so a same-second/same-size rewrite can never serve stale cached data. The limitation is documented next to the helper, and the _etag column is documented for the HDFS engine and table function.

Also fix a leak in tryGetObjectMetadata: the raw hdfsFileInfo * was freed manually after building the metadata, so an exception in between would leak it. Use the existing HDFSFileInfo RAII wrapper instead.

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Support _etag virtual column for HDFS storage

Version info

  • Merged into: 26.8.1.181 (included in 26.8 and later)

HDFS has no native ETag, so synthesise a version identifier from the
fields exposed by `hdfsFileInfo`: the modification time and the size.
Populate it in both `tryGetObjectMetadata` and `listObjects` so the
`_etag` virtual column, `use_parquet_metadata_cache`, the filesystem
cache and the page cache become usable on HDFS (previously they were
silently disabled because the etag was empty).

The token has only second precision: libhdfs3 truncates the NameNode
millisecond mtime to seconds in `mLastMod`, so a rewrite within the same
wall-clock second that keeps the exact same size collides. This is
acceptable for the virtual column and mirrors the mtime-based versioning
used for local files; the limitation is documented next to the helper.

Also fix a leak in `tryGetObjectMetadata`: the raw `hdfsFileInfo *` was
freed manually after building the metadata, so an exception in between
would leak it. Use the existing `HDFSFileInfo` RAII wrapper instead.

Co-authored-by: Cursor <cursoragent@cursor.com>
@tiandiwonder tiandiwonder self-assigned this Jun 23, 2026
@tiandiwonder tiandiwonder added the can be tested Allows running workflows for external contributors label Jun 23, 2026
@clickhouse-gh

clickhouse-gh Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [cf0bc35]

Summary:


AI Review

Summary

This PR makes HDFS populate ObjectMetadata::etag from (mtime_seconds, size), marks that token as weak so _etag becomes user-visible on StorageHDFS / hdfs() without enabling Parquet metadata, filesystem, or page-cache keys, and adds focused unit coverage for the weak-etag contract. I did not find a remaining correctness, safety, or compatibility issue in the current head.

Final Verdict
  • Status: ✅ Approve

@clickhouse-gh clickhouse-gh Bot added the pr-improvement Pull request with some product improvements label Jun 23, 2026
Comment thread src/Disks/DiskObjectStorage/ObjectStorages/HDFS/HDFSObjectStorage.cpp Outdated
Comment thread src/Disks/DiskObjectStorage/ObjectStorages/HDFS/HDFSObjectStorage.cpp Outdated
…document the `_etag` virtual column

Co-authored-by: Cursor <cursoragent@cursor.com>
Comment thread src/Disks/DiskObjectStorage/ObjectStorages/HDFS/HDFSObjectStorage.cpp Outdated

@tiandiwonder tiandiwonder left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No test for the new behavior. Worth adding a focused one: assert SELECT _etag FROM hdfs(...) is non-empty, and ideally that an HDFS read does not engage the parquet metadata cache (since etag_is_strong is false) while strong-etag storage still does. Otherwise a regression that re-empties the token or flips a cache gate goes uncaught. tests/integration/test_storage_hdfs/ can host it.

@tiandiwonder

Copy link
Copy Markdown
Contributor

The description still says this makes `use_parquet_metadata_cache`, the filesystem cache and the page cache usable on HDFS, but the code now sets `etag_is_strong = false`, so those caches stay disabled for HDFS — only the `_etag` virtual column is actually enabled. Worth updating the description and the changelog entry to reflect just the `_etag` column.

…g` token never keys a cache, with unit tests for the contract

Co-authored-by: Cursor <cursoragent@cursor.com>
@zhangyifan27

Copy link
Copy Markdown
Contributor Author

No test for the new behavior. Worth adding a focused one: assert SELECT _etag FROM hdfs(...) is non-empty, and ideally that an HDFS read does not engage the parquet metadata cache (since etag_is_strong is false) while strong-etag storage still does. Otherwise a regression that re-empties the token or flips a cache gate goes uncaught. tests/integration/test_storage_hdfs/ can host it.

Thanks — I agree the new behavior should be guarded against regressions.

The tests/integration/test_storage_hdfs/ directory is only a leftover stub, cluster.py no longer has a with_hdfs fixture and there is no HDFS docker-compose.

To make this testable without a NameNode I extracted the metadata construction into a pure static HDFSObjectStorage::makeObjectMetadata(mtime, size), which both call sites (tryGetObjectMetadata and listObjects) now use — so the tests exercise the same code path that runs in production.

@tiandiwonder

Copy link
Copy Markdown
Contributor

@zhangyifan27 please merge with latst master.

@clickhouse-gh

clickhouse-gh Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

LLVM Coverage Report

Metric Baseline Current Δ
Lines 85.80% 85.80% +0.00%
Functions 92.70% 92.70% +0.00%
Branches 78.00% 78.00% +0.00%

Changed lines: Changed C/C++ lines covered: 40/59 (67.80%) · Uncovered code

Full report · Diff report

@clickhouse-gh

clickhouse-gh Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

📊 Cloud Performance Report

✅ AI verdict: no_change — no significant changes across 36 queries analysed

This PR changes only HDFS object-storage metadata handling (a synthesized weak _etag and a new isEtagUsableAsCacheKey gate on the Parquet/page/filesystem caches). ClickBench queries run against local MergeTree data and never exercise that path, so the two flagged deltas — Q4 at -14.1% and Q15 at -17.5% — cannot be caused by this change. Both show essentially flat CPU (Q4 2655→2698 ms, Q15 2597→2599 ms) and land inside master's current-era variance band, so the deterministic no-change suppression is correct: these are run-to-run variance, not a PR effect. No real regressions or improvements to report.

clickbench

🟢 No significant changes

tpch_adapted_1_official

🟢 No significant changes

Debug info
  • StressHouse run: 6bfbcfa9-69c6-4292-9596-2267bcba615f
  • MIRAI run: 5c4be7f0-9daf-4412-894c-b4a50ba2f3a5
  • PR check IDs:
    • clickbench_112840_1784689034
    • clickbench_112853_1784689034
    • clickbench_112859_1784689034
    • tpch_adapted_1_official_112865_1784689034
    • tpch_adapted_1_official_112876_1784689034
    • tpch_adapted_1_official_112888_1784689034

@tiandiwonder
tiandiwonder added this pull request to the merge queue Jul 27, 2026
Merged via the queue into ClickHouse:master with commit a8c156e Jul 27, 2026
346 of 347 checks passed
@robot-ch-test-poll robot-ch-test-poll added the pr-synced-to-cloud The PR is synced to the cloud repo label Jul 27, 2026
@clickgapai

Copy link
Copy Markdown
Contributor

Hi @zhangyifan27 @tiandiwonder — while reviewing this PR I found the following:

Happy to discuss — close anything that's wrong or already addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

can be tested Allows running workflows for external contributors pr-improvement Pull request with some product improvements pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants