Skip to content

Antalya 25.8 - Backport of #99521 Parquet UUID support - #2446

Open
arthurpassos wants to merge 1 commit into
antalya-25.8from
backport/antalya-25.8/99521-arrow-parquet-uuid
Open

arthurpassos wants to merge 1 commit into
antalya-25.8from
backport/antalya-25.8/99521-arrow-parquet-uuid

Conversation

@arthurpassos

Copy link
Copy Markdown
Collaborator
  • Implements C++ reader/writer logic for UUIDs in Parquet/Arrow
  • Adds stateless test coverage using clickhouse-local to prevent CI collisions

Backport to antalya-25.8 of ClickHouse#99521.

Changelog category (leave one):

  • Improvement

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

Adds native support for importing and exporting UUID data types in Arrow and Parquet formats. Users can now directly query and transfer UUID data between ClickHouse and other data tools without requiring manual string conversions or workarounds. Automated logical inference for top-level UUIDs, and support for explicit schema hint for nested UUIDs.

Documentation entry for user-facing changes

...

CI/CD Options

Exclude tests:

  • Fast test
  • Integration Tests
  • Stateless tests
  • Stateful tests
  • Unit tests
  • Performance tests
  • Aarch64 tests
  • All with ASAN
  • All with TSAN
  • All with MSAN
  • All with UBSAN
  • All with Coverage
  • All Regression
  • Disable CI Cache

Regression jobs to run:

  • Fast suites (mostly <1h)
  • Aggregate Functions (2h)
  • Alter (1.5h)
  • Benchmark (30m)
  • CAS (content-addressed storage; Antalya only)
  • ClickHouse Keeper (1h)
  • Iceberg (2h)
  • LDAP (1h)
  • OAuth (5m)
  • Parquet (1.5h)
  • RBAC (1.5h)
  • SSL Server (1h)
  • S3 (2h)
  • S3 Export (2h)
  • Swarms (30m)
  • Tiered Storage (2h)

- Implements C++ reader/writer logic for UUIDs in Parquet/Arrow
- Adds stateless test coverage using clickhouse-local to prevent CI collisions

Backport to `antalya-25.8` of ClickHouse#99521.
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-29T23:15:08.414277Z 80463d6 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

Copy link
Copy Markdown

Workflow [PR], commit [80463d6]

@arthurpassos arthurpassos changed the title Add UUID inference and export support for Parquet and Arrow formats Antalya 25.8 - Backport of #99521 Parquet UUID support Sep 29, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 80463d6110

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".


struct ConverterUUID
{
using Statistics = StatisticsFixedStringRef;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Copy UUID statistics instead of retaining batch pointers

When a UUID page or column chunk is encoded from multiple batches, StatisticsFixedStringRef retains pointers into swapped_buf, but every subsequent getBatch() resizes and overwrites that buffer. Earlier extrema are therefore mutated or left dangling before page/column statistics are serialized, producing incorrect min/max values that can cause predicate pushdown to skip rows containing matches. Use owning fixed-size statistics storage for UUIDs rather than the reference-based implementation.

Useful? React with 👍 / 👎.

else
{
UUID res;
std::memcpy(&res, fixed_binary_array.GetValue(i), 16);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Validate UUID value buffers before reading them

For a malformed Arrow FixedSizeBinary chunk whose declared length exceeds its data buffer, the byte-width check still succeeds and GetValue(i) returns an address beyond the allocation, which this memcpy then reads. This newly added path also reserves from the unvalidated length first, unlike the existing fixed-binary readers; validate every chunk with the fixed-size buffer helper before reserving or accessing values so truncated Arrow input is rejected as INCORRECT_DATA rather than causing an out-of-bounds read.

Useful? React with 👍 / 👎.

@arthurpassos

Copy link
Copy Markdown
Collaborator Author

Grok:

Both comments describe code copied unchanged from the original pull request. The backport did not introduce either bug.

Statistics pointers. ConverterUUID in the original commit stores min/max as StatisticsFixedStringRef, which keeps raw pointers, and those pointers aim into swapped_buf. getBatch resizes and rewrites that buffer on every batch. A page is filled by calling getBatch repeatedly until data_page_size is reached, and only then does page_statistics.get copy the bytes. total_statistics.merge also keeps those pointers for the column-chunk stats. The backport's ConverterUUID is the same function. FixedString is safe with this statistics type because its pointers aim into the column's own data, which is not rewritten between batches.

Arrow bounds check. readColumnWithUUIDFromFixedBinaryData in the original commit reserves from arrow_column->length(), checks only byte_width() == 16, then memcpys from GetValue(i). There is no buffer-length check. The backport function is the same. The review's comparison with the other fixed-binary readers is about this branch: readColumnWithFixedStringData already calls validateChunksBeforeReserve before reserving. That helper was not in the file the original pull request changed, so the new UUID reader was never written to use it.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants