Repository navigation
Bound the stripe statistics index in startNextStripe and getStripeStatistics - #33
Merged
PedroTadim merged 2 commits intoOct 8, 2026
Conversation
…tistics A Metadata section with fewer entries than the footer has stripes (for example after a corrupt PostScript metadataLength) made RowReaderImpl::startNextStripe read stripe_stats(currentStripe_) out of bounds when a search argument was set (protobuf bounds assert on debug, SIGSEGV on release). Entries are matched to stripes only by position, so a list of a different length cannot be used at all: a too-small metadataLength keeps only the LAST entries, which would otherwise be applied to the first stripes. startNextStripe now evaluates stripe statistics only when there is one entry per stripe; otherwise it reads the stripe without stripe-level filtering (row group indexes, bloom filters and dictionaries still apply), as for a file with no Metadata. getStripeStatistics throws ParseError on a count mismatch and std::logic_error on an out-of-range stripe index. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 6, 2026
Merged
PedroTadim
reviewed
Oct 8, 2026
getStripeStatistics now rejects an out-of-range stripe index with orc::InvalidArgument (a std::runtime_error). ClickHouse treats any std::logic_error as a failed assertion and aborts in debug and sanitizer builds, so a caller-supplied index must not be reported that way. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
groeneai
added a commit
to groeneai/ClickHouse
that referenced
this pull request
Oct 8, 2026
Picks up ClickHouse/orc#33's second commit: getStripeStatistics reports an out-of-range stripe index with orc::InvalidArgument, which ClickHouse does not treat as a failed assertion. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Member
|
@groeneai can you create the same PR, but to the main branch? |
groeneai
added a commit
to groeneai/ClickHouse
that referenced
this pull request
Oct 8, 2026
Re-pin contrib/orc from the PR head 2c69dbd6 to 21955610, the merge commit of ClickHouse/orc#33 on ClickHouse/2.3.1. Both commits have the same tree (8783f43c), so the library sources are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Author
|
Done: #34 (same change on |
PedroTadim
pushed a commit
that referenced
this pull request
Oct 8, 2026
…tistics Same change as #33 on ClickHouse/2.3.1 (2195561). A Metadata section with fewer entries than the footer has stripes (for example after a corrupt PostScript metadataLength) made RowReaderImpl::startNextStripe read stripe_stats(currentStripe_) out of bounds when a search argument was set. Entries are matched to stripes only by position, so startNextStripe now evaluates stripe statistics only when there is one entry per stripe; otherwise it reads the stripe without stripe-level filtering, as for a file with no Metadata. getStripeStatistics throws ParseError on a count mismatch and InvalidArgument on an out-of-range stripe index. On main it reads stripe_stats(stripeIndex) before the stripe footer, so both checks go right after the metadata check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
RowReaderImpl::startNextStripeevaluated the stripe statistics underif (sargsApplier_ && contents_->metadata)and readstripe_stats(currentStripe_)without checking the size. It now evaluates them only whenstripe_stats_size() == stripes_size(); otherwise the stripes are read without stripe-level filtering, as for a file with no Metadata at all (row group indexes, bloom filters and dictionaries still apply). Entries are matched to stripes only by position, so a list of another length cannot be used: a too-smallmetadataLengthkeeps only the last entries, and a per-index check would apply them to the first stripes and drop matching rows.ReaderImpl::getStripeStatisticshas to return one specific entry, so before it reads the stripe footer it throwsParseError("Malformed metadata: the file has N stripes but M stripe statistics") on a count mismatch, andInvalidArgument("stripe index out of range") for an index past the last stripe.mainandfix-orc2-mainhave the same two unchecked reads; I can open the same change againstmainif you want it mirrored there too.Why are the changes needed?
A Metadata section can parse into fewer entries than the footer has stripes: in ClickHouse/ClickHouse#124238 a corrupt PostScript
metadataLengthmakes it parse as an empty message. Reading such a file with a search argument hit the protobuf bounds check on debug builds and crashed with SIGSEGV on release builds.How was this patch tested?
In ClickHouse, ClickHouse/ClickHouse#124315 adds a stateless test with two crafted uncompressed files: 10 rows with an empty Metadata, and two 10-row stripes whose
metadataLengthkeeps only the second stripe's entry. The current pin aborts instartNextStripeon both; this commit returns the same rows as with filter pushdown disabled, while a per-index check returns no rows for the second file. AnindexHintquery on the second file shows that the row group index still skips its second stripe. The issue's reproducer reads 1994 rows either way. A small harness callinggetStripeStatisticsthrows theParseErroron the short files andInvalidArgumentfor index 1 of a one-stripe file, and returns the statistics of valid one- and two-stripe files.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 5.5