-
Notifications
You must be signed in to change notification settings - Fork 4.2k
GH-43660: [C++] Add a CastingGenerator to Parquet Reader that applies required casts before slicing
#43661
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
GH-43660: [C++] Add a CastingGenerator to Parquet Reader that applies required casts before slicing
#43661
Changes from all commits
f3d31c0
c47a9ec
6fc1aaa
60747bf
8156cb7
e1d5d7f
019f795
f1a4955
6444d1f
cc30058
d850c50
4862a14
3d84d83
4939f5b
9c80447
05b717d
7e78f28
4de5587
cb4286f
b25b5c6
2353bfc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -26,11 +26,17 @@ | |||||
|
|
||||||
| #include "arrow/compute/cast.h" | ||||||
| #include "arrow/compute/exec.h" | ||||||
| #include "arrow/dataset/dataset.h" | ||||||
| #include "arrow/dataset/dataset_internal.h" | ||||||
| #include "arrow/dataset/parquet_encryption_config.h" | ||||||
| #include "arrow/dataset/scanner.h" | ||||||
| #include "arrow/filesystem/path_util.h" | ||||||
| #include "arrow/memory_pool.h" | ||||||
| #include "arrow/record_batch.h" | ||||||
| #include "arrow/result.h" | ||||||
| #include "arrow/table.h" | ||||||
| #include "arrow/type.h" | ||||||
| #include "arrow/type_fwd.h" | ||||||
| #include "arrow/util/checked_cast.h" | ||||||
| #include "arrow/util/future.h" | ||||||
| #include "arrow/util/iterator.h" | ||||||
|
|
@@ -565,6 +571,66 @@ Future<std::shared_ptr<parquet::arrow::FileReader>> ParquetFileFormat::GetReader | |||||
| }); | ||||||
| } | ||||||
|
|
||||||
| struct CastingGenerator { | ||||||
| CastingGenerator(RecordBatchGenerator source, std::shared_ptr<Schema> final_schema, | ||||||
| const std::unordered_set<std::string>& cols_to_skip, | ||||||
| MemoryPool* pool = default_memory_pool()) | ||||||
| : source_(source), | ||||||
| final_schema_(std::move(final_schema)), | ||||||
| cols_to_skip_(cols_to_skip), | ||||||
| exec_ctx(std::make_shared<compute::ExecContext>(pool)) {} | ||||||
|
|
||||||
| Future<std::shared_ptr<RecordBatch>> operator()() { | ||||||
| return this->source_().Then([this](const std::shared_ptr<RecordBatch>& next) | ||||||
| -> Result<std::shared_ptr<RecordBatch>> { | ||||||
| if (IsIterationEnd(next) || this->final_schema_ == nullptr) { | ||||||
| return next; | ||||||
| } | ||||||
| std::vector<std::shared_ptr<Array>> out_cols; | ||||||
| std::vector<std::shared_ptr<Field>> out_schema_fields; | ||||||
|
|
||||||
| bool changed = false; | ||||||
| for (const auto& field : this->final_schema_->fields()) { | ||||||
| FieldRef field_ref = FieldRef(field->name()); | ||||||
| ARROW_ASSIGN_OR_RAISE(std::shared_ptr<Array> column, | ||||||
| field_ref.GetOneOrNone(*next)); | ||||||
| if (column) { | ||||||
| if (this->cols_to_skip_.count(field->name())) { | ||||||
| // Maintain the original input type. | ||||||
| out_schema_fields.emplace_back(field->WithType(column->type())); | ||||||
| out_cols.emplace_back(std::move(column)); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, to skip the cast and leave them as they are. |
||||||
| continue; | ||||||
| } | ||||||
| if (!column->type()->Equals(field->type())) { | ||||||
| // Referenced field was present but didn't have the expected type. | ||||||
| ARROW_ASSIGN_OR_RAISE( | ||||||
| auto converted, | ||||||
| compute::Cast(column, field->type(), compute::CastOptions::Safe(), | ||||||
| this->exec_ctx.get())); | ||||||
| column = converted.make_array(); | ||||||
| changed = true; | ||||||
| } | ||||||
| out_schema_fields.emplace_back(field->Copy()); | ||||||
| out_cols.emplace_back(std::move(column)); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| if (changed) { | ||||||
| return RecordBatch::Make(std::make_shared<Schema>(std::move(out_schema_fields), | ||||||
| next->schema()->metadata()), | ||||||
| next->num_rows(), std::move(out_cols)); | ||||||
| } else { | ||||||
| return next; | ||||||
| } | ||||||
| }); | ||||||
| } | ||||||
|
|
||||||
| RecordBatchGenerator source_; | ||||||
| std::shared_ptr<Schema> final_schema_; | ||||||
| const std::unordered_set<std::string>& cols_to_skip_; | ||||||
| std::shared_ptr<compute::ExecContext> exec_ctx; | ||||||
| }; | ||||||
|
|
||||||
| struct SlicingGenerator { | ||||||
| SlicingGenerator(RecordBatchGenerator source, int64_t batch_size) | ||||||
| : state(std::make_shared<State>(source, batch_size)) {} | ||||||
|
|
@@ -627,6 +693,9 @@ Result<RecordBatchGenerator> ParquetFileFormat::ScanBatchesAsync( | |||||
| [this, options, parquet_fragment, pre_filtered, | ||||||
| row_groups](const std::shared_ptr<parquet::arrow::FileReader>& reader) mutable | ||||||
| -> Result<RecordBatchGenerator> { | ||||||
| // Since we already do the batching through the SlicingGenerator, we don't need the | ||||||
| // reader to batch its output. | ||||||
| reader->set_batch_size(std::numeric_limits<int64_t>::max()); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, if you have a very large row group (say 1 GB or more), what this will do is read the entire file at once, then slice it, instead of generating batches progressively. So this sounds like a bad idea to me. The entire purpose of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on my understanding of this example, wouldn't calling The goal of this PR was to reduce the overhead of casting batches of string offset buffers (which would require O(N^2) memory/compute where N is the length of a row group) by performing the cast on the entire table/row-group at once, which we will have at some point because of However, I understand wanting to keep the intended semantics of
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the explanation @scott-routledge2 . |
||||||
| // Ensure that parquet_fragment has FileMetaData | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assuming a large file, would memory usage grows high in this case?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we have that problem regardless of the batch size of the reader. arrow/cpp/src/parquet/arrow/reader.cc Line 1204 in 6a2e19a
FetchNext for RowGroupGenerator, it essentially calls ReadOneRowGroup which reads the entire row group into a table and then creates an output stream using TableBatchReader (arrow/cpp/src/parquet/arrow/reader.cc Line 1170 in 6a2e19a
batch_size just creates a reader on top of it which generates zero-copy slices. This is the same as what the SlicingGenerator does and is redundant. In my opinion, it's better to set the batch size to INT_MAX so that it returns one table per row group, and then we can perform the batching through the SlicingGenerator.
|
||||||
| RETURN_NOT_OK(parquet_fragment->EnsureCompleteMetadata(reader.get())); | ||||||
| if (!pre_filtered) { | ||||||
|
|
@@ -649,8 +718,17 @@ Result<RecordBatchGenerator> ParquetFileFormat::ScanBatchesAsync( | |||||
| ARROW_ASSIGN_OR_RAISE(auto generator, reader->GetRecordBatchGenerator( | ||||||
| reader, row_groups, column_projection, | ||||||
| cpu_executor, rows_to_readahead)); | ||||||
| // We need to skip casting the dictionary columns since the dataset_schema doesn't | ||||||
| // have the dictionary-encoding information. Parquet reader will return them with the | ||||||
| // dictionary type, which is what we eventually want. | ||||||
|
Comment on lines
+721
to
+723
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've forget something here, would
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasn't sure, so I left that case untouched. There are more casts done further up the chain (e.g. in |
||||||
| const std::unordered_set<std::string>& dict_cols = | ||||||
| parquet_fragment->parquet_format_.reader_options.dict_columns; | ||||||
| // Casting before slicing is more efficient. Casts on slices might require wasteful | ||||||
| // allocations and computation. | ||||||
|
Comment on lines
+726
to
+727
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain which kind of wasteful allocations and computations would be required when casting a slice?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When casting the string offsets buffer, we allocate
|
||||||
| RecordBatchGenerator casted = CastingGenerator( | ||||||
| std::move(generator), options->dataset_schema, dict_cols, options->pool); | ||||||
| RecordBatchGenerator sliced = | ||||||
| SlicingGenerator(std::move(generator), options->batch_size); | ||||||
| SlicingGenerator(std::move(casted), options->batch_size); | ||||||
| if (batch_readahead == 0) { | ||||||
| return sliced; | ||||||
| } | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| // Licensed to the Apache Software Foundation (ASF) under one | ||
| // or more contributor license agreements. See the NOTICE file | ||
| // distributed with this work for additional information | ||
| // regarding copyright ownership. The ASF licenses this file | ||
| // to you under the Apache License, Version 2.0 (the | ||
| // "License"); you may not use this file except in compliance | ||
| // with the License. You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, | ||
| // software distributed under the License is distributed on an | ||
| // "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| // KIND, either express or implied. See the License for the | ||
| // specific language governing permissions and limitations | ||
| // under the License. | ||
|
|
||
| #include "arrow/testing/gtest_util.h" | ||
| #include "benchmark/benchmark.h" | ||
|
|
||
| #include "arrow/api.h" | ||
| #include "arrow/dataset/file_parquet.h" | ||
| #include "arrow/io/memory.h" | ||
| #include "parquet/arrow/writer.h" | ||
|
|
||
| namespace arrow { | ||
| namespace dataset { | ||
|
|
||
| using parquet::arrow::WriteTable; | ||
|
|
||
| Result<std::shared_ptr<Buffer>> WriteStringColParquetBuffer(int64_t nrows) { | ||
| auto schema = arrow::schema({arrow::field("my_string_col", arrow::utf8())}); | ||
|
|
||
| arrow::StringBuilder builder; | ||
| for (int64_t i = 0; i < nrows; i++) { | ||
| ARROW_RETURN_NOT_OK(builder.Append("row_" + std::to_string(i))); | ||
| } | ||
| std::shared_ptr<arrow::Array> arr; | ||
| ARROW_RETURN_NOT_OK(builder.Finish(&arr)); | ||
| auto table = arrow::Table::Make(schema, {arr}); | ||
|
|
||
| ARROW_ASSIGN_OR_RAISE(auto sink, arrow::io::BufferOutputStream::Create()); | ||
| ARROW_RETURN_NOT_OK(WriteTable(*table, arrow::default_memory_pool(), sink)); | ||
| return sink->Finish(); | ||
| } | ||
|
|
||
| static void ParquetScanToTableCastStrings(benchmark::State& state) { | ||
| // GH-43660: Scan parquet data including a String column using a dataset object with | ||
| // LargeString in schema. | ||
| size_t num_batches = state.range(0); | ||
| size_t batch_size = state.range(1); | ||
| size_t nrows = num_batches * batch_size; | ||
| auto format = std::make_shared<ParquetFileFormat>(); | ||
|
|
||
| // Create a buffer with a single String column and wrap with FileFragment | ||
| ASSERT_OK_AND_ASSIGN(std::shared_ptr<Buffer> buffer, | ||
| WriteStringColParquetBuffer(nrows)); | ||
| auto buffer_reader = std::make_shared<arrow::io::BufferReader>(buffer); | ||
| FileSource source(buffer_reader, buffer->size()); | ||
| ASSERT_OK_AND_ASSIGN(auto fragment, format->MakeFragment(source)); | ||
| std::vector<std::shared_ptr<FileFragment>> fragments{fragment}; | ||
|
|
||
| // Create a dataset from FileFragment and set schema to LargeString (require casting). | ||
| auto schema = arrow::schema({field("my_string_col", arrow::large_utf8())}); | ||
| ASSERT_OK_AND_ASSIGN(auto dataset, FileSystemDataset::Make( | ||
| schema, compute::literal(true), format, | ||
| /*filesystem=*/nullptr, std::move(fragments))); | ||
|
|
||
| ASSERT_OK_AND_ASSIGN(auto builder, dataset->NewScan()); | ||
| ASSERT_OK(builder->BatchSize(batch_size)); | ||
| ASSERT_OK_AND_ASSIGN(auto scanner, builder->Finish()); | ||
|
|
||
| for (auto _ : state) { | ||
| ASSERT_OK_AND_ASSIGN(auto table, scanner->ToTable()); | ||
| benchmark::DoNotOptimize(table); | ||
| } | ||
|
|
||
| state.SetItemsProcessed(state.iterations() * nrows); | ||
| } | ||
|
|
||
| static void ParquetScanBenchmark_Customize(benchmark::internal::Benchmark* b) { | ||
| std::vector<int64_t> num_batches = {1000, 100, 10}; | ||
| std::vector<int64_t> batch_sizes = {1000, 10000, 100000}; | ||
|
|
||
| for (size_t i = 0; i < batch_sizes.size(); i++) { | ||
| b->Args({(num_batches[i]), batch_sizes[i]}); | ||
| } | ||
| b->ArgNames({"num_batches", "batch_size"}); | ||
| } | ||
|
|
||
| BENCHMARK(ParquetScanToTableCastStrings)->Apply(ParquetScanBenchmark_Customize); | ||
|
|
||
| } // namespace dataset | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Local results (comparing to main): |
||
| } // namespace arrow | ||
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'm a bit curious why casting generator is required, since during reading parquet to arrow, Parquet reader already applies a round of casting.
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.
You're probably right and it likely does perform some casts during the read. However, it doesn't seem to be doing it for certain cases like String to LargeString.
Could you point me to where the Parquet Reader should be performing this cast? I'm happy to add it there.
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.
https://github.com/apache/arrow/blob/main/cpp/src/parquet/arrow/reader_internal.cc#L474
Refer to here
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.
Thank you!
Based on what I see, that is only responsible for casting the data to the logical type specified in the parquet metadata and not the Arrow type we want to convert to (the one in the dataset_schema). For strings, that seems to always map to a String type (based on
FromByteArraywhich is called byGetArrowTypewhich is called byGetTypeForNodewhich is called byNodeToSchemaFieldwhich is called inSchemaManifest::Makeduring the creation of theLeafReader). Am I missing something?Uh oh!
There was an error while loading. Please reload this page.
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.
Parquet logical type doesn't have an arrow schema, isn't it? Binary reader reads from
::arrow::BinaryBuilder, and casting it to user-specified binary type.Yeah, you're right, the read "cast" with file-schema rather than an expected schema. I think a native cast is better here but this doesn't solve your problem, perhaps I can trying to add a naive
SchemaManifestwith hint solving here, but it would spend some time.Maybe we should rethink the
GetTypeForNodehandling for string/large_string/stringView, or using some handle written type hint here. A casting generator is also good for me when the reader cannot provide the right castingThere 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.
Provide hint would looks like: apache/arrow-rs#5939
Maybe I can add separate issue for that
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.
As far as I understand, the parquet metadata may or may not have the arrow schema. I believe it depends on the writer. It looks like it tries to get that using
GetOriginSchemainSchemaManifest::Make. However, the schema at write time might not be the same as the schema the reader expects.Sorry, I didn't quite follow. Are you saying that we should use this to do the cast at read time somehow?
That makes sense to me.
That would be great, thanks!
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 just check this. We can first add a
CastingGenerator, because sometimes we would have schema evolution here, and need cast to a "final type". The Parquet reader code can be regard as an optimizationThere 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.
Sounds good, thanks!