-
Notifications
You must be signed in to change notification settings - Fork 487
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ORC-1858: [C++] Add support to get StripeStatistics without row index #2144
Conversation
@wushap Gently ping, could you make the CI happy? |
c++/src/Reader.cc
Outdated
throw std::logic_error("No stripe statistics in file"); | ||
} | ||
StatContext statContext(hasCorrectStatistics()); | ||
return std::unique_ptr<Statistics>(new StatisticsImpl( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::make_unique would be better
Thanks for adding this! Could you please create a JIRA issue for this? If you don't have one, please request one from https://selfserve.apache.org/jira-account.html. |
|
c++/include/orc/Reader.hh
Outdated
* @param stripeIndex the index of the stripe (0 to N-1) to get statistics about | ||
* @return the statistics about that stripe without reading row group index statistics | ||
*/ | ||
virtual std::unique_ptr<Statistics> getStripeStatisticsOnly(uint64_t stripeIndex) const = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not to introduce a new API, especially when the name of getStripeStatisticsOnly
is a little bit misleading. This is not your fault, getStripeStatistics
does not clearly state it carries row index statistics as well :(
What about changing the existing API like below:
virtual std::unique_ptr<StripeStatistics> getStripeStatistics(uint64_t stripeIndex, bool includeRowIndex = true) const = 0;
Then return different StripeStatistics
subclasses depending on the value of includeRowIndex
.
class StripeStatisticsImpl : public StripeStatistics {
private:
std::unique_ptr<StatisticsImpl> columnStats_;
// DELIBERATELY NOT IMPLEMENTED
StripeStatisticsImpl(const StripeStatisticsImpl&);
StripeStatisticsImpl& operator=(const StripeStatisticsImpl&);
public:
StripeStatisticsImpl(const proto::StripeStatistics& stripeStats,
const StatContext& statContext);
virtual const ColumnStatistics* getColumnStatistics(uint32_t columnId) const override {
return columnStats_->getColumnStatistics(columnId);
}
uint32_t getNumberOfColumns() const override {
return columnStats_->getNumberOfColumns();
}
virtual const ColumnStatistics* getRowIndexStatistics(uint32_t columnId,
uint32_t rowIndex) const override {
throw NotImplementedYet("getRowIndexStatistics");
}
virtual ~StripeStatisticsImpl() override;
uint32_t getNumberOfRowIndexStats(uint32_t columnId) const override {
throw NotImplementedYet("getNumberOfRowIndexStats");
}
};
class StripeStatisticsWithRowIndexImpl : public StripeStatisticsImpl {
private:
std::vector<std::vector<std::shared_ptr<const ColumnStatistics> > > rowIndexStats_;
public:
StripeStatisticsWithRowIndexImpl(const proto::StripeStatistics& stripeStats,
std::vector<std::vector<proto::ColumnStatistics> >& indexStats,
const StatContext& statContext);
virtual const ColumnStatistics* getRowIndexStatistics(uint32_t columnId,
uint32_t rowIndex) const override {
// check id indices are valid
return rowIndexStats_[columnId][rowIndex].get();
}
virtual ~StripeStatisticsWithRowIndexImpl() override;
uint32_t getNumberOfRowIndexStats(uint32_t columnId) const override {
return static_cast<uint32_t>(rowIndexStats_[columnId].size());
}
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Thanks @wushap!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What changes were proposed in this pull request?
add a C++ API in reader to get stripe level statistics without reading row group index.
Why are the changes needed?
To #2137
How was this patch tested?
UT PASS
Was this patch authored or co-authored using generative AI tooling?
NO