You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "marin-ma (via GitHub)" <gi...@apache.org> on 2025/04/30 18:57:03 UTC
[PR] [GLUTEN-9468][VL] Remove parquet-arrow dependency [incubator-gluten]
marin-ma opened a new pull request, #9483:
URL: https://github.com/apache/incubator-gluten/pull/9483
Replace parquet-arrow writer with velox parquet writer to dump input for microbenchmark.
Refactor the code for dumping input for microbenchmark.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-9468][VL] Remove parquet-arrow dependency [incubator-gluten]
Posted by "marin-ma (via GitHub)" <gi...@apache.org>.
marin-ma commented on code in PR #9483:
URL: https://github.com/apache/incubator-gluten/pull/9483#discussion_r2071443682
##########
cpp/velox/operators/reader/ParquetReaderIterator.cc:
##########
@@ -39,11 +39,11 @@ std::shared_ptr<facebook::velox::common::ScanSpec> makeScanSpec(const facebook::
ParquetReaderIterator::ParquetReaderIterator(
const std::string& path,
int64_t batchSize,
- facebook::velox::memory::MemoryPool* pool)
- : FileReaderIterator(path), pool_(pool), batchSize_(batchSize) {}
+ std::shared_ptr<facebook::velox::memory::MemoryPool> pool)
+ : FileReaderIterator(path), pool_(std::move(pool)), batchSize_(batchSize) {}
void ParquetReaderIterator::createRowReader() {
- facebook::velox::dwio::common::ReaderOptions readerOptions{pool_};
+ facebook::velox::dwio::common::ReaderOptions readerOptions{pool_.get()};
Review Comment:
https://github.com/apache/incubator-gluten/pull/9483/files#diff-921be51ce29eb20bd569880bbcc0d450ff1529e78cbf4d101bc31bf37c9d0291R126-R127
The reader now owns a child memory pool. With this change we can track the memory usage separately in case the dumping causes OOM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-9468][VL] Remove parquet-arrow dependency [incubator-gluten]
Posted by "marin-ma (via GitHub)" <gi...@apache.org>.
marin-ma merged PR #9483:
URL: https://github.com/apache/incubator-gluten/pull/9483
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-9468][VL] Remove parquet-arrow dependency [incubator-gluten]
Posted by "zhouyuan (via GitHub)" <gi...@apache.org>.
zhouyuan commented on PR #9483:
URL: https://github.com/apache/incubator-gluten/pull/9483#issuecomment-2847254631
@marin-ma
it looks like arrow-parquet is not used anymore, so we could drop the arrow-parquet component when building arrow?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-9468][VL] Remove parquet-arrow dependency [incubator-gluten]
Posted by "marin-ma (via GitHub)" <gi...@apache.org>.
marin-ma commented on PR #9483:
URL: https://github.com/apache/incubator-gluten/pull/9483#issuecomment-2844354222
@jinchengchenghh @Yohahaha Could you help to review? Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-9468][VL] Remove parquet-arrow dependency [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #9483:
URL: https://github.com/apache/incubator-gluten/pull/9483#issuecomment-2842991392
https://github.com/apache/incubator-gluten/issues/9468
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-9468][VL] Remove parquet-arrow dependency [incubator-gluten]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on code in PR #9483:
URL: https://github.com/apache/incubator-gluten/pull/9483#discussion_r2070121354
##########
cpp/velox/operators/reader/ParquetReaderIterator.cc:
##########
@@ -39,11 +39,11 @@ std::shared_ptr<facebook::velox::common::ScanSpec> makeScanSpec(const facebook::
ParquetReaderIterator::ParquetReaderIterator(
const std::string& path,
int64_t batchSize,
- facebook::velox::memory::MemoryPool* pool)
- : FileReaderIterator(path), pool_(pool), batchSize_(batchSize) {}
+ std::shared_ptr<facebook::velox::memory::MemoryPool> pool)
+ : FileReaderIterator(path), pool_(std::move(pool)), batchSize_(batchSize) {}
void ParquetReaderIterator::createRowReader() {
- facebook::velox::dwio::common::ReaderOptions readerOptions{pool_};
+ facebook::velox::dwio::common::ReaderOptions readerOptions{pool_.get()};
Review Comment:
Why change the pool to std::shared_ptr<facebook::velox::memory::MemoryPool> pool
##########
cpp/core/jni/JniCommon.cc:
##########
@@ -91,6 +88,8 @@ gluten::JniColumnarBatchIterator::JniColumnarBatchIterator(
getMethodIdOrError(env, serializedColumnarBatchIteratorClass_, "hasNext", "()Z");
serializedColumnarBatchIteratorNext_ = getMethodIdOrError(env, serializedColumnarBatchIteratorClass_, "next", "()J");
jColumnarBatchItr_ = env->NewGlobalRef(jColumnarBatchItr);
+
+ shouldDump_ = runtime->getDumper() != nullptr;
Review Comment:
make shouldDump_ a const member
##########
cpp/velox/benchmarks/GenericBenchmark.cc:
##########
@@ -457,33 +457,34 @@ auto BM_Generic = [](::benchmark::State& state,
ArrowSchema cSchema;
toArrowSchema(veloxPlan->outputType(), runtime->memoryManager()->getLeafMemoryPool().get(), &cSchema);
GLUTEN_ASSIGN_OR_THROW(auto outputSchema, arrow::ImportSchema(&cSchema));
- auto writer = std::make_shared<VeloxArrowWriter>(
- FLAGS_save_output, FLAGS_batch_size, runtime->memoryManager()->getLeafMemoryPool().get());
+
+ auto writer = std::make_shared<VeloxColumnarBatchWriter>(
+ FLAGS_save_output, FLAGS_batch_size, runtime->memoryManager()->getAggregateMemoryPool());
+
state.PauseTiming();
- if (!FLAGS_save_output.empty()) {
- GLUTEN_THROW_NOT_OK(writer->initWriter(*(outputSchema.get())));
- }
+
state.ResumeTiming();
while (resultIter->hasNext()) {
- auto array = resultIter->next()->exportArrowArray();
- state.PauseTiming();
- auto maybeBatch = arrow::ImportRecordBatch(array.get(), outputSchema);
- if (!maybeBatch.ok()) {
- state.SkipWithError(maybeBatch.status().message().c_str());
- return;
+ auto cb = resultIter->next();
+ if (!FLAGS_save_output.empty()) {
+ GLUTEN_THROW_NOT_OK(writer->write(cb));
}
if (FLAGS_print_result) {
+ auto array = cb->exportArrowArray();
Review Comment:
Do we still have ArrowBatch? Maybe print RowVector by vector->toString(0, 20);
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org