Skip to content

Commit 003fe45

Browse files
cbb330cbb330
committed
[C++][ORC] Reconcile stats and reader signatures with Parquet style
Hard-replace ORC statistics null-count naming, switch scalar conversion to a Status + out-params signature, and align ORC GetRecordBatchReader ownership with parquet::arrow::FileReader by returning unique_ptr readers. Made-with: Cursor
1 parent ce71a4e commit 003fe45

6 files changed

Lines changed: 71 additions & 89 deletions

File tree

‎cpp/src/arrow/adapters/orc/adapter.cc‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -565,7 +565,7 @@ class ORCFileReader::Impl {
565565
pool_);
566566
}
567567

568-
Result<std::shared_ptr<RecordBatchReader>> GetRecordBatchReader(
568+
Result<std::unique_ptr<RecordBatchReader>> GetRecordBatchReader(
569569
int64_t batch_size, const std::vector<std::string>& include_names) {
570570
liborc::RowReaderOptions opts = DefaultRowReaderOptions();
571571
if (!include_names.empty()) {
@@ -578,7 +578,7 @@ class ORCFileReader::Impl {
578578
row_reader = reader_->createRowReader(opts);
579579
ORC_END_CATCH_NOT_OK
580580

581-
return std::make_shared<OrcStripeReader>(std::move(row_reader), schema, batch_size,
581+
return std::make_unique<OrcStripeReader>(std::move(row_reader), schema, batch_size,
582582
pool_);
583583
}
584584

@@ -680,7 +680,7 @@ Result<std::shared_ptr<RecordBatchReader>> ORCFileReader::NextStripeReader(
680680
return impl_->NextStripeReader(batch_size);
681681
}
682682

683-
Result<std::shared_ptr<RecordBatchReader>> ORCFileReader::GetRecordBatchReader(
683+
Result<std::unique_ptr<RecordBatchReader>> ORCFileReader::GetRecordBatchReader(
684684
int64_t batch_size, const std::vector<std::string>& include_names) {
685685
return impl_->GetRecordBatchReader(batch_size, include_names);
686686
}
@@ -741,10 +741,6 @@ std::string ORCFileReader::GetSerializedFileTail() {
741741
Result<FileMetaData> ORCFileReader::GetFileMetaData() { return impl_->GetFileMetaData(); }
742742

743743
const ::orc::Type& ORCFileReader::GetORCType() { return impl_->GetORCType(); }
744-
Result<std::shared_ptr<OrcSchemaManifest>> ORCFileReader::BuildSchemaManifest(
745-
const std::shared_ptr<Schema>& arrow_schema) const {
746-
return impl_->BuildSchemaManifest(arrow_schema);
747-
}
748744

749745
namespace {
750746

‎cpp/src/arrow/adapters/orc/adapter.h‎

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,7 @@ class ARROW_EXPORT ORCFileReader {
198198
/// \param[in] include_names the selected field names to read, if not empty
199199
/// (otherwise all fields are read)
200200
/// \return the record batch iterator
201-
Result<std::shared_ptr<RecordBatchReader>> GetRecordBatchReader(
201+
Result<std::unique_ptr<RecordBatchReader>> GetRecordBatchReader(
202202
int64_t batch_size, const std::vector<std::string>& include_names);
203203

204204
/// \brief The number of stripes in the file
@@ -297,18 +297,6 @@ class ARROW_EXPORT ORCFileReader {
297297
/// \brief Get file-level metadata view.
298298
Result<FileMetaData> GetFileMetaData();
299299

300-
/// \brief Build a schema manifest mapping Arrow fields to ORC column IDs.
301-
///
302-
/// Walks the ORC type tree paired with the Arrow schema to build a mapping
303-
/// from Arrow field paths to ORC physical column indices, which are needed
304-
/// for statistics lookup. ORC uses depth-first pre-order numbering where
305-
/// column 0 is the root struct.
306-
///
307-
/// \param[in] arrow_schema the Arrow schema (from ReadSchema())
308-
/// \return the built manifest or an error
309-
Result<std::shared_ptr<OrcSchemaManifest>> BuildSchemaManifest(
310-
const std::shared_ptr<Schema>& arrow_schema) const;
311-
312300
/// \brief Get the ORC type tree for column ID mapping.
313301
///
314302
/// \return reference to the root ORC Type (STRUCT), owned by the reader.

‎cpp/src/arrow/adapters/orc/adapter_test.cc‎

Lines changed: 32 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -368,6 +368,23 @@ Result<std::vector<adapters::orc::Statistics>> GetStripeColumnStatisticsBulk(
368368
return stats;
369369
}
370370

371+
struct ScalarStats {
372+
bool has_null;
373+
int64_t num_values;
374+
bool has_min_max;
375+
std::shared_ptr<Scalar> min;
376+
std::shared_ptr<Scalar> max;
377+
};
378+
379+
Result<ScalarStats> ToScalarStats(const adapters::orc::Statistics& statistics) {
380+
ScalarStats converted;
381+
converted.has_null = statistics.HasNullCount();
382+
converted.num_values = statistics.num_values();
383+
RETURN_NOT_OK(adapters::orc::StatisticsAsScalars(statistics, &converted.min, &converted.max));
384+
converted.has_min_max = converted.min != nullptr && converted.max != nullptr;
385+
return converted;
386+
}
387+
371388
TEST(TestAdapterRead, FileMetaDataViewAccessors) {
372389
MemoryOutputStream mem_stream(kDefaultMemStreamSize);
373390
std::unique_ptr<liborc::Type> type(
@@ -1302,7 +1319,7 @@ TEST(TestAdapterRead, GetColumnStatisticsInteger) {
13021319
adapters::orc::ORCFileReader::Open(in_stream, default_memory_pool()));
13031320

13041321
ASSERT_OK_AND_ASSIGN(auto col1_stats, GetFileColumnStatistics(reader.get(), 1));
1305-
ASSERT_OK_AND_ASSIGN(auto col1_scalars, adapters::orc::StatisticsAsScalars(col1_stats));
1322+
ASSERT_OK_AND_ASSIGN(auto col1_scalars, ToScalarStats(col1_stats));
13061323
EXPECT_EQ(col1_scalars.num_values, row_count);
13071324
EXPECT_TRUE(col1_scalars.has_min_max);
13081325
ASSERT_NE(col1_scalars.min, nullptr);
@@ -1311,7 +1328,7 @@ TEST(TestAdapterRead, GetColumnStatisticsInteger) {
13111328
EXPECT_EQ(checked_pointer_cast<Int64Scalar>(col1_scalars.max)->value, 999);
13121329

13131330
ASSERT_OK_AND_ASSIGN(auto col2_stats, GetFileColumnStatistics(reader.get(), 2));
1314-
ASSERT_OK_AND_ASSIGN(auto col2_scalars, adapters::orc::StatisticsAsScalars(col2_stats));
1331+
ASSERT_OK_AND_ASSIGN(auto col2_scalars, ToScalarStats(col2_stats));
13151332
EXPECT_EQ(col2_scalars.num_values, row_count);
13161333
EXPECT_TRUE(col2_scalars.has_min_max);
13171334
ASSERT_NE(col2_scalars.min, nullptr);
@@ -1351,8 +1368,7 @@ TEST(TestAdapterRead, GetStripeColumnStatistics) {
13511368

13521369
ASSERT_OK_AND_ASSIGN(auto stripe0_stats,
13531370
GetStripeColumnStatistics(reader.get(), 0, 1));
1354-
ASSERT_OK_AND_ASSIGN(auto stripe0_scalars,
1355-
adapters::orc::StatisticsAsScalars(stripe0_stats));
1371+
ASSERT_OK_AND_ASSIGN(auto stripe0_scalars, ToScalarStats(stripe0_stats));
13561372
EXPECT_TRUE(stripe0_scalars.has_min_max);
13571373
EXPECT_EQ(checked_pointer_cast<Int64Scalar>(stripe0_scalars.min)->value, 100);
13581374
EXPECT_EQ(checked_pointer_cast<Int64Scalar>(stripe0_scalars.max)->value, 599);
@@ -1397,7 +1413,7 @@ TEST(TestAdapterRead, GetColumnStatisticsString) {
13971413
adapters::orc::ORCFileReader::Open(in_stream, default_memory_pool()));
13981414

13991415
ASSERT_OK_AND_ASSIGN(auto col_stats, GetFileColumnStatistics(reader.get(), 1));
1400-
ASSERT_OK_AND_ASSIGN(auto col_scalars, adapters::orc::StatisticsAsScalars(col_stats));
1416+
ASSERT_OK_AND_ASSIGN(auto col_scalars, ToScalarStats(col_stats));
14011417
EXPECT_EQ(col_scalars.num_values, row_count);
14021418
EXPECT_TRUE(col_scalars.has_min_max);
14031419
ASSERT_NE(col_scalars.min, nullptr);
@@ -1474,8 +1490,8 @@ TEST(TestAdapterRead, GetColumnStatisticsWithNulls) {
14741490
adapters::orc::ORCFileReader::Open(in_stream, default_memory_pool()));
14751491

14761492
ASSERT_OK_AND_ASSIGN(auto col_stats, GetFileColumnStatistics(reader.get(), 1));
1477-
ASSERT_OK_AND_ASSIGN(auto col_scalars, adapters::orc::StatisticsAsScalars(col_stats));
1478-
EXPECT_TRUE(col_stats.has_null());
1493+
ASSERT_OK_AND_ASSIGN(auto col_scalars, ToScalarStats(col_stats));
1494+
EXPECT_TRUE(col_stats.HasNullCount());
14791495
EXPECT_TRUE(col_scalars.has_min_max);
14801496
}
14811497

@@ -1510,7 +1526,7 @@ TEST(TestAdapterRead, GetColumnStatisticsBoolean) {
15101526
adapters::orc::ORCFileReader::Open(in_stream, default_memory_pool()));
15111527

15121528
ASSERT_OK_AND_ASSIGN(auto col_stats, GetFileColumnStatistics(reader.get(), 1));
1513-
ASSERT_OK_AND_ASSIGN(auto col_scalars, adapters::orc::StatisticsAsScalars(col_stats));
1529+
ASSERT_OK_AND_ASSIGN(auto col_scalars, ToScalarStats(col_stats));
15141530
EXPECT_EQ(col_scalars.num_values, row_count);
15151531
EXPECT_FALSE(col_scalars.has_min_max); // Boolean types don't have min/max
15161532
EXPECT_EQ(col_scalars.min, nullptr);
@@ -1548,7 +1564,7 @@ TEST(TestAdapterRead, GetColumnStatisticsDate) {
15481564
adapters::orc::ORCFileReader::Open(in_stream, default_memory_pool()));
15491565

15501566
ASSERT_OK_AND_ASSIGN(auto col_stats, GetFileColumnStatistics(reader.get(), 1));
1551-
ASSERT_OK_AND_ASSIGN(auto col_scalars, adapters::orc::StatisticsAsScalars(col_stats));
1567+
ASSERT_OK_AND_ASSIGN(auto col_scalars, ToScalarStats(col_stats));
15521568
EXPECT_EQ(col_scalars.num_values, row_count);
15531569
EXPECT_TRUE(col_scalars.has_min_max);
15541570
ASSERT_NE(col_scalars.min, nullptr);
@@ -1589,7 +1605,7 @@ TEST(TestAdapterRead, GetColumnStatisticsTimestamp) {
15891605
adapters::orc::ORCFileReader::Open(in_stream, default_memory_pool()));
15901606

15911607
ASSERT_OK_AND_ASSIGN(auto col_stats, GetFileColumnStatistics(reader.get(), 1));
1592-
ASSERT_OK_AND_ASSIGN(auto col_scalars, adapters::orc::StatisticsAsScalars(col_stats));
1608+
ASSERT_OK_AND_ASSIGN(auto col_scalars, ToScalarStats(col_stats));
15931609
EXPECT_EQ(col_scalars.num_values, row_count);
15941610
EXPECT_TRUE(col_scalars.has_min_max);
15951611
ASSERT_NE(col_scalars.min, nullptr);
@@ -1880,7 +1896,7 @@ TEST(TestAdapterRead, GetColumnStatisticsDoubleNaN) {
18801896
adapters::orc::ORCFileReader::Open(in_stream, default_memory_pool()));
18811897

18821898
ASSERT_OK_AND_ASSIGN(auto col_stats, GetFileColumnStatistics(reader.get(), 1));
1883-
ASSERT_OK_AND_ASSIGN(auto col_scalars, adapters::orc::StatisticsAsScalars(col_stats));
1899+
ASSERT_OK_AND_ASSIGN(auto col_scalars, ToScalarStats(col_stats));
18841900
// When NaN values are present, ORC may report NaN as min or max.
18851901
// Our guard should detect this and set has_min_max = false.
18861902
if (col_scalars.has_min_max) {
@@ -1982,11 +1998,9 @@ TEST(TestAdapterRead, GetStripeStatisticsBulk) {
19821998
ASSERT_OK_AND_ASSIGN(
19831999
auto individual_stats,
19842000
GetStripeColumnStatistics(reader.get(), 0, column_indices[i]));
1985-
ASSERT_OK_AND_ASSIGN(auto bulk_scalars,
1986-
adapters::orc::StatisticsAsScalars(bulk_stats[i]));
1987-
ASSERT_OK_AND_ASSIGN(auto individual_scalars,
1988-
adapters::orc::StatisticsAsScalars(individual_stats));
1989-
EXPECT_EQ(bulk_stats[i].has_null(), individual_stats.has_null());
2001+
ASSERT_OK_AND_ASSIGN(auto bulk_scalars, ToScalarStats(bulk_stats[i]));
2002+
ASSERT_OK_AND_ASSIGN(auto individual_scalars, ToScalarStats(individual_stats));
2003+
EXPECT_EQ(bulk_stats[i].HasNullCount(), individual_stats.HasNullCount());
19902004
EXPECT_EQ(bulk_scalars.num_values, individual_scalars.num_values);
19912005
EXPECT_EQ(bulk_scalars.has_min_max, individual_scalars.has_min_max);
19922006
if (bulk_scalars.has_min_max && individual_scalars.has_min_max) {
@@ -2042,9 +2056,9 @@ TEST(TestAdapterRead, GetColumnStatisticsDecimal) {
20422056
adapters::orc::ORCFileReader::Open(in_stream, default_memory_pool()));
20432057

20442058
ASSERT_OK_AND_ASSIGN(auto col_stats, GetFileColumnStatistics(reader.get(), 1));
2045-
ASSERT_OK_AND_ASSIGN(auto col_scalars, adapters::orc::StatisticsAsScalars(col_stats));
2059+
ASSERT_OK_AND_ASSIGN(auto col_scalars, ToScalarStats(col_stats));
20462060
EXPECT_EQ(col_scalars.num_values, row_count);
2047-
EXPECT_FALSE(col_stats.has_null());
2061+
EXPECT_FALSE(col_stats.HasNullCount());
20482062
EXPECT_TRUE(col_scalars.has_min_max);
20492063
ASSERT_NE(col_scalars.min, nullptr);
20502064
ASSERT_NE(col_scalars.max, nullptr);

‎cpp/src/arrow/adapters/orc/statistics.cc‎

Lines changed: 28 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ bool MillisFitInNanos(int64_t millis) {
4444

4545
} // namespace
4646

47-
bool Statistics::has_null() const { return column_statistics_->hasNull(); }
47+
bool Statistics::HasNullCount() const { return column_statistics_->hasNull(); }
4848

4949
int FileMetaData::num_columns() const {
5050
return static_cast<int>(file_statistics_->getNumberOfColumns());
@@ -201,50 +201,49 @@ bool Statistics::HasMinMax() const {
201201
return false;
202202
}
203203

204-
Result<ColumnStatisticsAsScalars> StatisticsAsScalars(const Statistics& statistics) {
204+
Status StatisticsAsScalars(const Statistics& statistics, std::shared_ptr<Scalar>* min,
205+
std::shared_ptr<Scalar>* max) {
206+
if (min == nullptr || max == nullptr) {
207+
return Status::Invalid("Output pointers for min and max cannot be null");
208+
}
205209
if (!statistics.valid()) {
206210
return Status::Invalid("ORC statistics wrapper is not initialized");
207211
}
208212

209-
ColumnStatisticsAsScalars converted;
210-
converted.has_null = statistics.has_null();
211-
converted.num_values = statistics.num_values();
212-
converted.has_min_max = false;
213-
converted.min = nullptr;
214-
converted.max = nullptr;
213+
*min = nullptr;
214+
*max = nullptr;
215215

216216
if (!statistics.HasMinMax()) {
217-
return converted;
217+
return Status::OK();
218218
}
219219

220-
converted.has_min_max = true;
221220
if (const auto* int_stats = statistics.integer()) {
222-
converted.min = std::make_shared<Int64Scalar>(int_stats->getMinimum());
223-
converted.max = std::make_shared<Int64Scalar>(int_stats->getMaximum());
224-
return converted;
221+
*min = std::make_shared<Int64Scalar>(int_stats->getMinimum());
222+
*max = std::make_shared<Int64Scalar>(int_stats->getMaximum());
223+
return Status::OK();
225224
}
226225
if (const auto* double_stats = statistics.floating_point()) {
227-
converted.min = std::make_shared<DoubleScalar>(double_stats->getMinimum());
228-
converted.max = std::make_shared<DoubleScalar>(double_stats->getMaximum());
229-
return converted;
226+
*min = std::make_shared<DoubleScalar>(double_stats->getMinimum());
227+
*max = std::make_shared<DoubleScalar>(double_stats->getMaximum());
228+
return Status::OK();
230229
}
231230
if (const auto* string_stats = statistics.string()) {
232-
converted.min = std::make_shared<StringScalar>(string_stats->getMinimum());
233-
converted.max = std::make_shared<StringScalar>(string_stats->getMaximum());
234-
return converted;
231+
*min = std::make_shared<StringScalar>(string_stats->getMinimum());
232+
*max = std::make_shared<StringScalar>(string_stats->getMaximum());
233+
return Status::OK();
235234
}
236235
if (const auto* date_stats = statistics.date()) {
237-
converted.min = std::make_shared<Date32Scalar>(date_stats->getMinimum());
238-
converted.max = std::make_shared<Date32Scalar>(date_stats->getMaximum());
239-
return converted;
236+
*min = std::make_shared<Date32Scalar>(date_stats->getMinimum());
237+
*max = std::make_shared<Date32Scalar>(date_stats->getMaximum());
238+
return Status::OK();
240239
}
241240
if (const auto* ts_stats = statistics.timestamp()) {
242241
auto ts_type = timestamp(TimeUnit::NANO);
243-
converted.min = std::make_shared<TimestampScalar>(
242+
*min = std::make_shared<TimestampScalar>(
244243
ts_stats->getMinimum() * 1000000LL + ts_stats->getMinimumNanos(), ts_type);
245-
converted.max = std::make_shared<TimestampScalar>(
244+
*max = std::make_shared<TimestampScalar>(
246245
ts_stats->getMaximum() * 1000000LL + ts_stats->getMaximumNanos(), ts_type);
247-
return converted;
246+
return Status::OK();
248247
}
249248
if (const auto* decimal_stats = statistics.decimal()) {
250249
liborc::Decimal min_dec = decimal_stats->getMinimum();
@@ -254,15 +253,12 @@ Result<ColumnStatisticsAsScalars> StatisticsAsScalars(const Statistics& statisti
254253
Decimal128 max_d128(max_dec.value.getHighBits(), max_dec.value.getLowBits());
255254
auto dec_type = decimal128(38, min_dec.scale);
256255

257-
converted.min = std::make_shared<Decimal128Scalar>(min_d128, dec_type);
258-
converted.max = std::make_shared<Decimal128Scalar>(max_d128, dec_type);
259-
return converted;
256+
*min = std::make_shared<Decimal128Scalar>(min_d128, dec_type);
257+
*max = std::make_shared<Decimal128Scalar>(max_d128, dec_type);
258+
return Status::OK();
260259
}
261260

262-
converted.has_min_max = false;
263-
converted.min = nullptr;
264-
converted.max = nullptr;
265-
return converted;
261+
return Status::OK();
266262
}
267263

268264
} // namespace orc

‎cpp/src/arrow/adapters/orc/statistics.h‎

Lines changed: 5 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323

2424
#include "arrow/result.h"
2525
#include "arrow/scalar.h"
26+
#include "arrow/status.h"
2627
#include "arrow/type_fwd.h"
2728
#include "arrow/util/visibility.h"
2829

@@ -49,20 +50,6 @@ class Statistics;
4950
class ColumnMetaData;
5051
class StripeMetaData;
5152

52-
/// \brief Scalar materialization of ORC column statistics.
53-
struct ARROW_EXPORT ColumnStatisticsAsScalars {
54-
/// \brief Whether the column contains null values.
55-
bool has_null;
56-
/// \brief Number of non-null values in the column.
57-
int64_t num_values;
58-
/// \brief Whether min/max statistics are available and valid.
59-
bool has_min_max;
60-
/// \brief Minimum value (nullptr if unavailable).
61-
std::shared_ptr<Scalar> min;
62-
/// \brief Maximum value (nullptr if unavailable).
63-
std::shared_ptr<Scalar> max;
64-
};
65-
6653
/// \brief File-level ORC metadata container.
6754
class ARROW_EXPORT FileMetaData {
6855
public:
@@ -122,7 +109,7 @@ class ARROW_EXPORT Statistics {
122109
: column_statistics_(column_statistics) {}
123110

124111
bool valid() const { return column_statistics_ != nullptr; }
125-
bool has_null() const;
112+
bool HasNullCount() const;
126113
std::optional<int64_t> null_count() const;
127114
int64_t num_values() const;
128115
bool HasMinMax() const;
@@ -156,8 +143,9 @@ class ARROW_EXPORT ColumnMetaData {
156143
Statistics statistics_;
157144
};
158145

159-
ARROW_EXPORT Result<ColumnStatisticsAsScalars> StatisticsAsScalars(
160-
const Statistics& statistics);
146+
ARROW_EXPORT Status StatisticsAsScalars(const Statistics& statistics,
147+
std::shared_ptr<Scalar>* min,
148+
std::shared_ptr<Scalar>* max);
161149

162150
} // namespace orc
163151
} // namespace adapters

‎cpp/src/arrow/dataset/file_orc.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ class OrcScanTask {
8585
included_fields.push_back(schema->field(match.indices()[0])->name());
8686
}
8787

88-
std::shared_ptr<RecordBatchReader> record_batch_reader;
88+
std::unique_ptr<RecordBatchReader> record_batch_reader;
8989
ARROW_ASSIGN_OR_RAISE(
9090
record_batch_reader,
9191
reader->GetRecordBatchReader(scan_options.batch_size, included_fields));
@@ -99,7 +99,7 @@ class OrcScanTask {
9999
return batch;
100100
}
101101

102-
std::shared_ptr<RecordBatchReader> record_batch_reader_;
102+
std::unique_ptr<RecordBatchReader> record_batch_reader_;
103103
};
104104

105105
return Impl::Make(fragment_->source(),

0 commit comments

Comments
 (0)