felipepessoto commented on code in PR #13136:
URL: https://github.com/apache/gluten/pull/13136#discussion_r4320763975
##########
cpp/velox/tests/Substrait2VeloxPlanConversionTest.cc:
##########
@@ -64,12 +82,89 @@ class Substrait2VeloxPlanConversionTest : public
exec::test::HiveConnectorTestBa
if (splitInfo->columnMappingMode.has_value()) {
splitBuilder.columnMappingMode(*splitInfo->columnMappingMode);
}
+ for (const auto& [name, value] : splitInfo->metadataColumns.at(i)) {
+ splitBuilder.infoColumn(name, value);
+ }
auto split = splitBuilder.build();
+ EXPECT_EQ(split->columnMappingMode, splitInfo->columnMappingMode);
splits.emplace_back(split);
}
return splits;
}
+ ::substrait::ReadRel makeRead(
+ const RowTypePtr& type,
+ const std::vector<::substrait::NamedStruct::ColumnType>& columnTypes =
{}) {
+ google::protobuf::Arena arena;
+ VeloxToSubstraitTypeConvertor typeConverter;
+ ::substrait::ReadRel read;
+ read.mutable_common()->mutable_direct();
+
read.mutable_base_schema()->CopyFrom(typeConverter.toSubstraitNamedStruct(arena,
type));
+ for (auto columnType : columnTypes) {
+ read.mutable_base_schema()->add_column_types(columnType);
+ }
+ return read;
+ }
+
+ ::substrait::ReadRel_LocalFiles makeParquetFiles(
+ const std::vector<std::string>& paths,
+ const RowTypePtr& tableSchema = nullptr,
+ dwio::common::ColumnMappingMode columnMappingMode =
dwio::common::ColumnMappingMode::kName) {
+ ::substrait::ReadRel_LocalFiles files;
+ google::protobuf::Arena arena;
+ VeloxToSubstraitTypeConvertor typeConverter;
+ for (const auto& path : paths) {
+ auto* file = files.add_items();
+ file->set_uri_file(path);
+ file->set_length(std::numeric_limits<uint64_t>::max());
+ file->mutable_parquet();
+ auto* metadata = file->add_metadata_columns();
+ metadata->set_key("file_name");
+ metadata->set_value(std::filesystem::path(path).filename().string());
+ google::protobuf::StringValue mode;
+
mode.set_value(std::string(dwio::common::ColumnMappingModeName::toName(columnMappingMode)));
+ auto* mapping = file->add_other_const_metadata_columns();
+ mapping->set_key("__gluten.column_mapping_mode");
+ mapping->mutable_value()->PackFrom(mode);
+ if (tableSchema) {
+
file->mutable_schema()->CopyFrom(typeConverter.toSubstraitNamedStruct(arena,
tableSchema));
+ }
+ }
+ return files;
+ }
+
+ std::shared_ptr<const core::TableScanNode> convertRead(
+ const ::substrait::ReadRel& read,
+ const ::substrait::ReadRel_LocalFiles& files,
+ const std::vector<std::string>& functions = {}) {
+ ::substrait::Plan plan;
+ plan.add_relations()->mutable_rel()->mutable_read()->CopyFrom(read);
+ for (size_t i = 0; i < functions.size(); ++i) {
+ auto* function = plan.add_extensions()->mutable_extension_function();
+ function->set_function_anchor(static_cast<uint32_t>(i));
+ function->set_name(functions[i]);
+ }
+ planConverter_ = std::make_shared<VeloxPlanConverter>(
+ pool(),
+ veloxCfg_.get(),
+ std::vector<std::shared_ptr<ResultIterator>>{},
+ VeloxConnectorIds{.hive =
facebook::velox::exec::test::kHiveConnectorId});
+ auto scan = std::dynamic_pointer_cast<const
core::TableScanNode>(planConverter_->toVeloxPlan(plan, {files}));
+ VELOX_CHECK_NOT_NULL(scan);
+ return scan;
+ }
+
+ void writeParquet(const std::string& name, const RowVectorPtr& rows) {
+ const auto path = tmpDir_->getPath() + name;
+ auto file = std::make_unique<LocalWriteFile>(path, false, true);
+ auto sink = std::make_unique<dwio::common::WriteFileSink>(std::move(file),
path);
+ dwio::common::WriterOptions options;
+ options.memoryPool = rootPool_.get();
+ parquet::Writer writer(std::move(sink), options, asRowType(rows->type()));
+ writer.write(rows);
+ writer.close();
+ }
Review Comment:
Keeping the existing path handling. `makeSplits()` prepends
`tmpDir_->getPath()` before constructing each `HiveConnectorSplitBuilder`,
matching the path used by `writeParquet()`. All Parquet execution tests call
`.splits(makeSplits(scan))`; the `/unused.parquet` cases only convert plans and
never open files. The files are therefore read from the temporary directory,
not the filesystem root. This is the same point addressed in the earlier path
thread.
##########
cpp/velox/tests/Substrait2VeloxPlanConversionTest.cc:
##########
@@ -39,6 +46,17 @@ namespace gluten {
class Substrait2VeloxPlanConversionTest : public
exec::test::HiveConnectorTestBase {
protected:
+ void SetUp() override {
+ HiveConnectorTestBase::SetUp();
+ registerAllFunctions();
+ parquet::registerParquetReaderFactory();
+ }
+
+ void TearDown() override {
+ HiveConnectorTestBase::TearDown();
+ parquet::unregisterParquetReaderFactory();
+ }
Review Comment:
Keeping the current teardown order. `HiveConnectorTestBase::TearDown()`
drains the I/O executor, and `OperatorTestBase::TearDown()` waits for tasks and
releases their resources. Leaving the Parquet factory registered until
afterward avoids unregistering it while pending work can still use it. A scope
guard could separately harden cleanup against exceptions, but that does not
justify unregistering before the base teardown; no such failure has been
demonstrated here.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
