Copilot commented on code in PR #11712:
URL: https://github.com/apache/gluten/pull/11712#discussion_r3480609773
##########
cpp/velox/utils/VeloxWriterUtils.h:
##########
@@ -23,7 +23,7 @@
namespace gluten {
-std::unique_ptr<facebook::velox::parquet::WriterOptions>
makeParquetWriteOption(
+std::unique_ptr<facebook::velox::dwio::common::WriterOptions>
makeParquetWriteOption(
const std::unordered_map<std::string, std::string>& sparkConfs);
Review Comment:
The header now declares makeParquetWriteOption() returning
dwio::common::WriterOptions, but the implementation in
cpp/velox/utils/VeloxWriterUtils.cc still defines it as returning
parquet::WriterOptions (via `using namespace facebook::velox::parquet;` and
`std::unique_ptr<WriterOptions>`). This is a conflicting declaration and will
fail to compile; the implementation needs to be migrated to build a
dwio::common::WriterOptions (and move parquet-specific fields into
formatSpecificOptions) or the declaration/call sites need to be reverted
consistently.
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -878,7 +878,7 @@ core::PlanNodePtr
SubstraitToVeloxPlanConverter::toVeloxPlan(const ::substrait::
GLUTEN_CHECK(formatShortName == "parquet", "Unsupported file write format: "
+ formatShortName);
dwio::common::FileFormat fileFormat = dwio::common::FileFormat::PARQUET;
- const std::shared_ptr<facebook::velox::parquet::WriterOptions> writerOptions
= makeParquetWriteOption(writeConfs);
+ const std::shared_ptr<facebook::velox::dwio::common::WriterOptions>
writerOptions = makeParquetWriteOption(writeConfs);
// Spark's default compression code is snappy.
const auto& compressionKind =
writerOptions->compressionKind.value_or(common::CompressionKind::CompressionKind_SNAPPY);
Review Comment:
This call site was updated to treat makeParquetWriteOption() as producing
dwio::common::WriterOptions, but makeParquetWriteOption’s current
implementation still constructs parquet::WriterOptions (and sets many
parquet-only fields). With the new Velox API, those parquet-specific settings
likely need to be placed in WriterOptions::formatSpecificOptions (e.g.,
ParquetWriterOptions), otherwise parquet write behavior/config will be lost or
compilation will fail once the types are corrected.
--
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]
