Workflow [PR], commit [423ed0f]

Summary: ✅

AI Review

1) Summary

This PR fixes a real correctness issue in background Parquet reads by switching metadata-cache access from CurrentThread::getQueryContext() to the explicit Context passed through FormatFactory, and adds a targeted regression test for S3Queue + Parquet v3 + metadata cache. High-level verdict: the change is small, focused, and correct; I found no blockers or majors.

2) Missing context (if any)

• Full CI has not completed yet at review time (only early workflow artifacts/check states were available).
• No dedicated performance benchmark results were provided (likely unnecessary for this small fix, but still absent).

3) Findings (by severity)

Blockers

• None.

Majors

• None.

The PR looks good.

4) Tests & Evidence

• Positive coverage: new stateless test 04034_parquet_v3_metadata_cache_no_query_context.sh reproduces the background-thread path (S3Queue) and validates rows are fully ingested (count=100, sum=4950).
• Regression relevance: test specifically exercises the previously failing context access pattern with input_format_parquet_use_native_reader_v3=1 and use_parquet_metadata_cache=1.
• Negative/error-path coverage: no explicit negative assertions were added (e.g., cache disabled / missing metadata), but the added regression test is appropriately targeted to the fixed path.
• Suggested optional follow-up tests: add one case with use_parquet_metadata_cache=0 for the same ingestion path to ensure non-cache behavior remains unchanged.

5) ClickHouse Compliance Checklist (Yes/No + short note)

• Data deletions logged? Yes — no new deletion paths introduced.
• Serialization formats versioned? Yes (N/A) — no serialization/protocol changes.
• Experimental setting gate present? Yes (N/A) — no new feature introduced.
• Settings exposed for constants/thresholds? Yes (N/A) — no new constants introduced in core logic.
• Backward compatibility preserved? Yes — behavior change is a bug fix in context lookup only.
• SettingsHistory.cpp updated for new/changed settings? Yes (N/A) — no settings added/changed.
• Existing tests untouched (only additions)? Yes — only one new test + reference added.
• Docs/user-facing notes updated? Yes (N/A) — internal bug fix with test; no user-facing syntax/behavior docs needed.
• Core-area change got extra scrutiny? Yes — reviewed FormatFactory plumbing and Parquet input path.

6) Performance & Safety Notes

• Hot-path impact is negligible: one pointer access source changed (CurrentThread query context -> explicit Context) without added loops/allocations.
• Safety improves in background threads by removing dependence on thread-local query context availability.
• No new concurrency primitives introduced; no additional memory/resource lifetime risks observed.
• Benchmarking: not provided; a minimal sanity benchmark could compare Parquet read throughput with metadata cache on/off for unchanged performance expectations.

7) User-Lens Review

• The fix is intuitive and robust: background consumers now use the correct context object instead of assuming query context exists.
• Operator experience should improve because this removes an exception-prone path in asynchronous ingestion threads.
• Logging/error behavior is unchanged and remains actionable.

8) Final Verdict

• Status: Approve

No tests

423ed0f

no explanation, no link to CI?

https://pastila.nl/?001a755c/6964bb8d5180dc5249a29874fd0605f2#KuZwHaHJq+YWh3d8naQn0Q==GCM

LLVM Coverage Report

PR changed lines: PR changed-lines coverage: 100.00% (21/21)
Diff coverage report
Uncovered code

Thanks for the test and the explanation!