Copilot commented on code in PR #12963:
URL: https://github.com/apache/gluten/pull/12963#discussion_r4072320648
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -1538,6 +1538,22 @@ core::PlanNodePtr
SubstraitToVeloxPlanConverter::toVeloxPlan(const ::substrait::
!readRel.common().has_emit(), "Emit not supported for ValuesNode and
TableScanNode related Substrait plans.");
}
+ // Virtual tables are self-contained and do not have Spark file splits.
+ if (readRel.has_virtual_table()) {
+ std::vector<TypePtr> veloxTypes;
+ if (readRel.has_base_schema()) {
+ const bool asLowerCase = !veloxCfg_->get<bool>(kCaseSensitive, false);
+ veloxTypes = SubstraitParser::parseNamedStruct(readRel.base_schema(),
asLowerCase);
+ }
+
+ std::vector<std::string> outputNames;
+ outputNames.reserve(veloxTypes.size());
+ for (int32_t idx = 0; idx < veloxTypes.size(); ++idx) {
+ outputNames.emplace_back(SubstraitParser::makeNodeName(planNodeId_,
idx));
+ }
+ return toVeloxPlan(readRel, ROW(std::move(outputNames),
std::move(veloxTypes)));
+ }
Review Comment:
`virtual_table` conversion allows `base_schema` to be absent, which can
yield `numColumns == 0` downstream. If the virtual table contains any fields,
later validation often relies on modulo/division by `numColumns`, which risks
undefined behavior (division by zero) or misinterpreting a non-empty table as
zero-column. Consider enforcing `readRel.has_base_schema()` for virtual tables
with any fields (or explicitly validating that all expressions have zero fields
when `base_schema` is missing) and fail fast with a clear `VELOX_USER_CHECK`
message.
##########
cpp/velox/tests/Substrait2VeloxValuesNodeConversionTest.cc:
##########
@@ -65,4 +68,208 @@ TEST_F(Substrait2VeloxValuesNodeConversionTest, valuesNode)
{
assertQuery(veloxPlan, "SELECT * FROM tmp");
}
+TEST_F(Substrait2VeloxValuesNodeConversionTest, zeroColumnOneRowValuesNode) {
+ auto planPath =
FilePathGenerator::getDataFilePath("substrait_virtualTable_emptySchema.json");
+
+ ::substrait::Plan substraitPlan;
+ JsonToProtoConverter::readFromFile(planPath, substraitPlan);
+ auto veloxCfg =
std::make_shared<facebook::velox::config::ConfigBase>(std::unordered_map<std::string,
std::string>());
Review Comment:
This test now directly uses `std::unordered_map` but does not include
`<unordered_map>`. Relying on transitive includes is non-portable and can break
with different standard library implementations or header refactors; add an
explicit `#include <unordered_map>` in this file.
##########
gluten-substrait/src/main/java/org/apache/gluten/substrait/rel/RelBuilder.java:
##########
@@ -171,6 +171,17 @@ public static RelNode makeReadRelForInputIterator(
return new InputIteratorRelNode(typeList, nameList, iteratorIndex);
}
+ public static RelNode makeVirtualTableReadRel(
+ List<TypeNode> types,
+ List<String> names,
+ List<List<Expression.Literal>> rows,
+ SubstraitContext context,
+ Long operatorId) {
+ RelNode node = new VirtualTableRelNode(types, names, rows);
+ context.registerRelToOperator(operatorId);
+ return node;
+ }
Review Comment:
`operatorId` is typed as boxed `Long`, which permits `null` and can produce
a late `NullPointerException` inside `registerRelToOperator`. Using a primitive
`long` here (and aligning with other builder APIs, if they use primitives)
makes nullability impossible by construction and improves call-site safety.
--
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]
