Copilot commented on code in PR #12963:
URL: https://github.com/apache/gluten/pull/12963#discussion_r4080834709
##########
backends-velox/src/test/scala/org/apache/gluten/execution/MiscOperatorSuite.scala:
##########
@@ -791,16 +798,322 @@ class MiscOperatorSuite extends
VeloxWholeStageTransformerSuite with AdaptiveSpa
}
test("test OneRowRelation") {
- val df = sql("SELECT 1")
- checkAnswer(df, Row(1))
- val plan = df.queryExecution.executedPlan
- if (isSparkVersionGE("4.1")) {
- assert(plan.find(_.getClass.getSimpleName ==
"OneRowRelationExec").isDefined)
- } else {
- assert(plan.find(_.isInstanceOf[RDDScanExec]).isDefined)
+ val testCases = Seq(
+ "SELECT 1" -> Seq(Row(1)),
+ "SELECT 'x', 42, CAST(NULL AS INT)" -> Seq(Row("x", 42, null)),
+ "SELECT (SELECT 1)" -> Seq(Row(1)),
+ "SELECT (SELECT 1), (SELECT 1)" -> Seq(Row(1, 1)),
+ "SELECT 1 UNION ALL SELECT 2" -> Seq(Row(1), Row(2))
+ )
+
+ def checkOneRowRelation(query: String, expected: Seq[Row]): Unit = {
+ val df = sql(query)
+ checkAnswer(df, expected)
+ assert(df.count() == expected.size)
Review Comment:
This runs two Spark actions (`checkAnswer` triggers a job and `count()`
triggers another) per query/config combination, which can significantly slow
the suite and may complicate metric-based assertions due to multiple
executions. A more efficient approach is to collect once (or reuse
`checkAnswer`’s results) and derive the row count from that single action.
##########
cpp/velox/tests/Substrait2VeloxValuesNodeConversionTest.cc:
##########
@@ -65,4 +69,231 @@ 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>());
+ auto planConverter = std::make_shared<SubstraitToVeloxPlanConverter>(
+ pool_.get(),
+ veloxCfg.get(),
+ std::vector<std::shared_ptr<ResultIterator>>{},
+ VeloxConnectorIds{},
+ std::nullopt,
+ std::nullopt,
+ false);
+ auto veloxPlan = planConverter->toVeloxPlan(substraitPlan);
+
+ auto valuesNode = std::dynamic_pointer_cast<const
core::ValuesNode>(veloxPlan);
+ ASSERT_NE(valuesNode, nullptr);
+ ASSERT_TRUE(valuesNode->outputType()->equivalent(*ROW({})));
+ ASSERT_EQ(valuesNode->values().size(), 1);
+ ASSERT_EQ(valuesNode->values().front()->childrenSize(), 0);
+ ASSERT_EQ(valuesNode->values().front()->size(), 1);
+ ASSERT_EQ(planConverter->splitInfos().at(valuesNode->id())->leafType,
SplitInfo::LeafType::TRIVIAL_LEAF);
Review Comment:
`splitInfos().at(valuesNode->id())` will throw `std::out_of_range` if the
key is missing, which turns an assertion into an unhandled exception. Prefer
asserting presence first (e.g., `count()`/`find()`) and then checking
`leafType`, so failures are reported as test assertions with clearer
diagnostics.
##########
cpp/velox/tests/Substrait2VeloxValuesNodeConversionTest.cc:
##########
@@ -49,7 +53,7 @@ TEST_F(Substrait2VeloxValuesNodeConversionTest, valuesNode) {
VeloxConnectorIds{},
std::nullopt,
std::nullopt,
- true);
+ false);
Review Comment:
This change drops coverage of the `valuesNode` test in the `validationMode =
true` path (it previously passed `true`). Consider running this test in both
modes (similar to the new virtual-table tests) to preserve validation-mode
coverage for the non-empty-schema values conversion.
--
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]
