Copilot commented on code in PR #13136:
URL: https://github.com/apache/gluten/pull/13136#discussion_r4319482771
##########
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:
`writeParquet()` always prefixes `tmpDir_->getPath()` to `name`. Several
tests pass values like `\"/first.parquet\"` into both `writeParquet()` and
`makeParquetFiles()`, but `makeParquetFiles()` will then reference
`\"/first.parquet\"` while the actual file is written to
`<tmpDir>/first.parquet`. This will make the execution tests try to read a
non-existent file. Fix by making the path construction consistent: either (a)
have tests pass the full path (e.g., returned by `writeParquet`), or (b) change
`writeParquet()` to accept absolute paths as-is (and only prefix/join for
relative names), ideally using `std::filesystem::path` joining instead of
string concatenation.
##########
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:
The Parquet reader factory is global process state. Unregistering it after
`HiveConnectorTestBase::TearDown()` increases the risk of leaving it registered
if the base teardown throws/ASSERTs, and can also keep the factory active
longer than needed between tests. Prefer unregistering the factory before
calling the base `TearDown()` (or use a small RAII guard owned by the fixture)
so cleanup is guaranteed and ordered.
##########
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();
Review Comment:
Setting `length` to `uint64_t::max()` can distort split sizing logic or any
code that uses the declared file length for planning/validation, and makes the
test less representative. Prefer setting `length` to the real on-disk size for
local test files (e.g., `std::filesystem::file_size(path)`), or omit it if the
field is optional in your converter path.
--
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]
