zhouyuan commented on code in PR #13236:
URL: https://github.com/apache/gluten/pull/13236#discussion_r4327947371
##########
cpp/velox/substrait/SubstraitToVeloxPlan.h:
##########
@@ -204,6 +209,11 @@ class SubstraitToVeloxPlanConverter {
splitInfos_ = splitInfos;
}
+ /// Serialized splits in scan traversal order, decoded according to each
ReadRel's scan type.
+ void setRawSplitInfos(std::vector<std::string> rawSplitInfos) {
+ rawSplitInfos_ = std::move(rawSplitInfos);
Review Comment:
why do we need to make a copy in plan converter here?
##########
cpp/velox/compute/VeloxRuntime.cc:
##########
@@ -404,9 +404,9 @@ void VeloxRuntime::parseSplitInfo(const uint8_t* data,
int32_t size, int32_t spl
LOG(WARNING) << "Error converting substrait::ReadRel.LocalFiles to JSON:
" << e.what();
}
}
- ::substrait::ReadRel_LocalFiles localFile;
- GLUTEN_CHECK(parseProtobuf(data, size, &localFile) == true, "Parse substrait
plan failed");
- localFiles_.push_back(localFile);
+ // The matching ReadRel determines whether these bytes encode LocalFiles or
StreamKafka.
+ // Decode them during plan conversion, after the scan type is known.
+ rawSplitInfos_.emplace_back(reinterpret_cast<const char*>(data), size);
Review Comment:
the method here seems tricky, could we explicitly make the type for
localfile and stream Kafka?
##########
cpp/velox/compute/VeloxPlanConverter.cc:
##########
@@ -270,9 +273,13 @@ void parseLocalFileNodes(
std::shared_ptr<const facebook::velox::core::PlanNode>
VeloxPlanConverter::toVeloxPlan(
const ::substrait::Plan& substraitPlan,
- std::vector<::substrait::ReadRel_LocalFiles> localFiles) {
+ std::vector<::substrait::ReadRel_LocalFiles> localFiles,
+ std::vector<std::string> rawSplitInfos) {
if (!validationMode_) {
- parseLocalFileNodes(&substraitVeloxPlanConverter_, veloxCfg_, localFiles);
+ if (rawSplitInfos.empty()) {
+ parseLocalFileNodes(&substraitVeloxPlanConverter_, veloxCfg_,
localFiles);
+ }
+ substraitVeloxPlanConverter_.setRawSplitInfos(std::move(rawSplitInfos));
Review Comment:
could you please add some note here?
##########
cpp/velox/tests/Substrait2VeloxPlanConversionTest.cc:
##########
@@ -29,6 +29,10 @@
#include "FilePathGenerator.h"
#include "compute/VeloxBackend.h"
+#include "kafka.pb.h"
+#ifdef ENABLE_KAFKA
Review Comment:
move this to the line above?
##########
cpp/velox/tests/Substrait2VeloxPlanConversionTest.cc:
##########
@@ -427,4 +431,131 @@ TEST_F(Substrait2VeloxPlanConversionTest,
aggregateMaskMustBeTopLevelField) {
"Aggregation Operator only supports a top-level field mask.");
}
+TEST_F(Substrait2VeloxPlanConversionTest, serializedFileSplit) {
Review Comment:
it's better to guard all these new tests under ENABLE_KAFKA
--
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]
