You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "luis4a0 (via GitHub)" <gi...@apache.org> on 2026/09/03 11:33:35 UTC
[PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
luis4a0 opened a new pull request, #12963:
URL: https://github.com/apache/gluten/pull/12963
## What changes are proposed in this pull request?
Spark 4.1 uses `OneRowRelationExec` as the physical source for zero-column,
one-row inputs in literal-only queries and scalar subqueries. The Velox backend
does not currently transform this leaf, so an otherwise native plan must retain
a Spark operator and cross a row-to-columnar boundary.
This PR adds native Spark 4.1 `OneRowRelationExec` support by:
- recognizing the operator in the Spark 4.1 shim without introducing a Spark
4.1 dependency into common code;
- replacing it only for the Velox backend while preserving the original Spark
plan for unsupported backends and Spark versions;
- representing the logical row as a zero-column, one-row Substrait virtual
table and converting it to a Velox `ValuesNode`;
- introducing an explicit, conservative no-input execution capability so a
supported dependency-free native stage receives exactly one Spark partition;
- propagating that capability only through safe unary, union, and streamed-side
broadcast-join paths;
- validating virtual-table schemas, row cardinality, null handling, malformed
values, and split-free native conversion; and
- documenting the supported virtual-table and `OneRowRelationExec` behavior.
This is needed for Spark 4.1 native-plan completeness. The existing Spark
fallback produces correct results, but it fragments literal-only and
scalar-subquery pipelines and leaves an avoidable execution boundary. The new
no-input behavior is deliberately opt-in: unrelated empty or broadcast-only
stages retain their previous failure behavior, and unsupported backends retain
their original Spark plans.
This PR is not presented as a performance optimization. Its purpose is to close
the Spark 4.1 capability gap with explicit execution semantics and regression
coverage.
## How was this patch tested?
Added and ran JVM unit, Spark integration, and native C++ tests covering:
- typed Spark 4.1 recognition and false-positive rejection;
- unsupported-backend identity preservation;
- Substrait virtual-table protobuf generation, batching, schema and null
validation, zero-column cardinality, and legal zero-row execution;
- dependency-free and broadcast-only partition discovery, including concurrent
access and unsupported-stage rejection;
- AQE enabled and disabled, scalar-subquery reuse, native union, broadcast hash
join, broadcast nested-loop join, and multi-partition file-scan coexistence;
- Spark plan-tag preservation, exact output rows, native-stage presence, and
removal of the OneRow-specific fallback boundary; and
- native `ValuesNode` execution, malformed and non-literal rejection,
trivial-leaf registration, and file-split isolation.
Results on the current `main` base:
- 14 focused `gluten-substrait` tests passed.
- 3 Spark 4.1 shim tests passed.
- All 106 executed `MiscOperatorSuite` tests passed; one existing test was
ignored.
- The Spark 4.1 explain regression passed.
- 52 native plan-conversion tests passed; 7 existing round-trip tests were
skipped.
- Spotless, Scalastyle, Checkstyle, `clang-format-15`, license-header, and
source-hygiene checks passed.
## Was this patch authored or co-authored using generative AI tooling?
Generated-by: GitHub Copilot CLI 1.0.83-0
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12963:
URL: https://github.com/apache/gluten/pull/12963#issuecomment-5829459502
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12963:
URL: https://github.com/apache/gluten/pull/12963#issuecomment-5525837742
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12963:
URL: https://github.com/apache/gluten/pull/12963#discussion_r4081958035
##########
cpp/velox/tests/Substrait2VeloxValuesNodeConversionTest.cc:
##########
@@ -62,7 +56,250 @@ TEST_F(Substrait2VeloxValuesNodeConversionTest, valuesNode) {
});
createDuckDbTable({expectedData});
- assertQuery(veloxPlan, "SELECT * FROM tmp");
+ for (const bool validationMode : {false, true}) {
+ 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,
+ validationMode);
+ assertQuery(planConverter->toVeloxPlan(substraitPlan), "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);
+ const auto& splitInfos = planConverter->splitInfos();
+ const auto splitInfo = splitInfos.find(valuesNode->id());
+ ASSERT_NE(splitInfo, splitInfos.end());
+ ASSERT_NE(splitInfo->second, nullptr);
+ ASSERT_EQ(splitInfo->second->leafType, SplitInfo::LeafType::TRIVIAL_LEAF);
+
+ CursorParameters params;
+ params.planNode = veloxPlan;
+ auto [cursor, results] = readCursor(params);
+ ASSERT_EQ(results.size(), 1);
+ ASSERT_EQ(results.front()->childrenSize(), 0);
+ ASSERT_EQ(results.front()->size(), 1);
+}
+
+TEST_F(Substrait2VeloxValuesNodeConversionTest, virtualTableDoesNotConsumeTableScanSplit) {
+ auto planPath = FilePathGenerator::getDataFilePath("substrait_virtualTable.json");
+
+ ::substrait::Plan substraitPlan;
+ JsonToProtoConverter::readFromFile(planPath, substraitPlan);
+ const auto virtualRead = substraitPlan.relations(0).root().input().read();
+
+ auto* setRel = substraitPlan.mutable_relations(0)->mutable_root()->mutable_input()->mutable_set();
+ setRel->set_op(::substrait::SetRel_SetOp::SetRel_SetOp_SET_OP_UNION_ALL);
+ setRel->add_inputs()->mutable_read()->CopyFrom(virtualRead);
+ auto* tableScanRead = setRel->add_inputs()->mutable_read();
+ tableScanRead->CopyFrom(virtualRead);
+ tableScanRead->clear_virtual_table();
+
+ auto veloxCfg = std::make_shared<facebook::velox::config::ConfigBase>(std::unordered_map<std::string, std::string>());
+ VeloxConnectorIds connectorIds;
+ connectorIds.hive = "test-hive";
+ auto planConverter = std::make_shared<SubstraitToVeloxPlanConverter>(
+ pool_.get(),
+ veloxCfg.get(),
+ std::vector<std::shared_ptr<ResultIterator>>{},
+ std::move(connectorIds),
+ std::nullopt,
+ std::nullopt,
+ false);
+ auto tableScanSplit = std::make_shared<SplitInfo>();
+ tableScanSplit->leafType = SplitInfo::LeafType::TABLE_SCAN;
+ planConverter->setSplitInfos({tableScanSplit});
+
+ auto veloxPlan = planConverter->toVeloxPlan(substraitPlan);
+ ASSERT_NE(std::dynamic_pointer_cast<const core::LocalPartitionNode>(veloxPlan), nullptr);
+
+ const auto& splitInfos = planConverter->splitInfos();
+ ASSERT_EQ(splitInfos.size(), 2);
+ ASSERT_EQ(
+ std::count_if(
+ splitInfos.begin(),
+ splitInfos.end(),
+ [](const auto& entry) { return entry.second->leafType == SplitInfo::LeafType::TRIVIAL_LEAF; }),
+ 1);
+ ASSERT_EQ(
+ std::count_if(
+ splitInfos.begin(), splitInfos.end(), [&](const auto& entry) { return entry.second == tableScanSplit; }),
+ 1);
+}
+
+TEST_F(Substrait2VeloxValuesNodeConversionTest, rejectsEmptyVirtualTableInAllModes) {
+ auto planPath = FilePathGenerator::getDataFilePath("substrait_virtualTable_emptySchema.json");
+ for (const bool validationMode : {false, true}) {
+ ::substrait::Plan substraitPlan;
+ JsonToProtoConverter::readFromFile(planPath, substraitPlan);
+ substraitPlan.mutable_relations(0)
+ ->mutable_root()
+ ->mutable_input()
+ ->mutable_read()
+ ->mutable_virtual_table()
+ ->clear_expressions();
+
+ 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,
+ validationMode);
+
+ VELOX_ASSERT_THROW(planConverter->toVeloxPlan(substraitPlan), "Virtual table must contain at least one row group.");
+ }
+}
+
+TEST_F(Substrait2VeloxValuesNodeConversionTest, supportsZeroRowVirtualTableInAllModes) {
+ auto planPath = FilePathGenerator::getDataFilePath("substrait_virtualTable.json");
+ for (const bool validationMode : {false, true}) {
+ ::substrait::Plan substraitPlan;
+ JsonToProtoConverter::readFromFile(planPath, substraitPlan);
+ substraitPlan.mutable_relations(0)
+ ->mutable_root()
+ ->mutable_input()
+ ->mutable_read()
+ ->mutable_virtual_table()
+ ->mutable_expressions(0)
+ ->clear_fields();
+
+ 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,
+ validationMode);
+
+ auto values =
+ std::dynamic_pointer_cast<const facebook::velox::core::ValuesNode>(planConverter->toVeloxPlan(substraitPlan));
+ ASSERT_NE(values, nullptr);
+ ASSERT_EQ(values->values().size(), 1);
+ ASSERT_EQ(values->values().front()->size(), 0);
+ if (!validationMode) {
+ CursorParameters params;
+ params.planNode = values;
+ auto [cursor, results] = readCursor(params);
Review Comment:
Same issue as above: `cursor` is bound but unused. This is prone to `-Wunused-variable` failures in stricter toolchains. Avoid binding `cursor` if you don't need it.
##########
cpp/velox/tests/Substrait2VeloxValuesNodeConversionTest.cc:
##########
@@ -62,7 +56,250 @@ TEST_F(Substrait2VeloxValuesNodeConversionTest, valuesNode) {
});
createDuckDbTable({expectedData});
- assertQuery(veloxPlan, "SELECT * FROM tmp");
+ for (const bool validationMode : {false, true}) {
+ 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,
+ validationMode);
+ assertQuery(planConverter->toVeloxPlan(substraitPlan), "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);
+ const auto& splitInfos = planConverter->splitInfos();
+ const auto splitInfo = splitInfos.find(valuesNode->id());
+ ASSERT_NE(splitInfo, splitInfos.end());
+ ASSERT_NE(splitInfo->second, nullptr);
+ ASSERT_EQ(splitInfo->second->leafType, SplitInfo::LeafType::TRIVIAL_LEAF);
+
+ CursorParameters params;
+ params.planNode = veloxPlan;
+ auto [cursor, results] = readCursor(params);
Review Comment:
The structured binding assigns `cursor` but it is never used in this test. If the build enables `-Wunused-variable` (often with `-Werror`), this can fail compilation. Consider restructuring to avoid binding an unused variable (e.g., bind to a single variable and access `.second`, or otherwise explicitly ignore the cursor).
##########
gluten-substrait/src/test/scala/org/apache/gluten/execution/WholeStageNoInputSuite.scala:
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gluten.execution
+
+import org.mockito.Mockito.mock
+import org.scalatest.funsuite.AnyFunSuite
+
+import java.util.concurrent.TimeUnit
+
+class WholeStageNoInputSuite extends AnyFunSuite {
+ test("explicit no-input execution creates one dependency-free partition") {
+ val wrapper = new ColumnarInputRDDsWrapper(Seq.empty, supportsNoInputExecution = true)
+
+ assert(wrapper.getPartitionLength == 1)
+ assert(wrapper.getDependencies.isEmpty)
+ assert(wrapper.getPartitions(0).isEmpty)
+ assert(wrapper.getIterators(Seq.empty, null).isEmpty)
Review Comment:
Passing `null` for the TaskContext parameter makes this test fragile and can mask real regressions if `getIterators` later starts using the context unconditionally. Prefer using a real `TaskContext` (e.g., the Spark-provided empty context if available) or a mock/stub to keep the test robust.
##########
shims/common/src/main/scala/org/apache/gluten/sql/shims/SparkShims.scala:
##########
@@ -173,6 +173,8 @@ trait SparkShims {
*/
def getLocalTableScanStream(plan: LocalTableScanExec): Option[SparkDataStream] = None
Review Comment:
`isOneRowRelationExec` is a new cross-version shim hook, but it has no Scaladoc explaining the Spark-version semantics (e.g., Spark 4.1+) and the intended usage/contract (what callers should assume when it returns false). Adding a brief comment here would make the API clearer for new shim implementers.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12963:
URL: https://github.com/apache/gluten/pull/12963#issuecomment-5732988179
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12963:
URL: https://github.com/apache/gluten/pull/12963#issuecomment-6443506749
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12963:
URL: https://github.com/apache/gluten/pull/12963#issuecomment-5795206396
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "luis4a0 (via GitHub)" <gi...@apache.org>.
luis4a0 commented on PR #12963:
URL: https://github.com/apache/gluten/pull/12963#issuecomment-6367322787
I also checked the existing Spark 4.1 group-3 failure:
https://github.com/apache/gluten/actions/runs/36113847460/job/108013706124
That run executed 8,738 tests, with one failure: "Cancellation APIs in SparkSession are isolated", where the job-ended counter was 1 rather than 2. The upstream test's onJobEnd releases its semaphore before incrementing that counter, so the waiting assertion can observe the preceding value. The jobs use range inputs, not OneRowRelationExec. The exact cancellation test passes in the focused rerun; I have not changed this unrelated test or claimed the historical CI run is green.
The OneRow serialization and integration regressions pass locally. Main-branch conflicts remain, so this follow-up alone does not make the PR merge-ready or establish fresh merge-CI success.
Upstream test source: https://github.com/apache/spark/blob/v4.1.1/sql/core/src/test/scala/org/apache/spark/sql/SparkSessionJobTaggingAndCancellationSuite.scala
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12963:
URL: https://github.com/apache/gluten/pull/12963#issuecomment-6371818062
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12963:
URL: https://github.com/apache/gluten/pull/12963#issuecomment-5525082594
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12963:
URL: https://github.com/apache/gluten/pull/12963#issuecomment-5531964078
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12963:
URL: https://github.com/apache/gluten/pull/12963#issuecomment-5792534578
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "luis4a0 (via GitHub)" <gi...@apache.org>.
luis4a0 commented on code in PR #12963:
URL: https://github.com/apache/gluten/pull/12963#discussion_r4080688763
##########
gluten-substrait/src/main/java/org/apache/gluten/substrait/rel/VirtualTableRelNode.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gluten.substrait.rel;
+
+import org.apache.gluten.substrait.type.TypeNode;
+import org.apache.gluten.utils.SubstraitUtil;
+
+import io.substrait.proto.Expression;
+import io.substrait.proto.NamedStruct;
+import io.substrait.proto.ReadRel;
+import io.substrait.proto.Rel;
+import io.substrait.proto.RelCommon;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Objects;
+
+public class VirtualTableRelNode implements RelNode, Serializable {
+ private final List<TypeNode> types;
+ private final List<String> names;
+ private final List<List<Expression.Literal>> rows;
+
+ VirtualTableRelNode(
+ List<TypeNode> types, List<String> names, List<List<Expression.Literal>> rows) {
+ Objects.requireNonNull(types, "types");
+ Objects.requireNonNull(names, "names");
+ Objects.requireNonNull(rows, "rows");
+ if (types.size() != names.size()) {
+ throw new IllegalArgumentException(
+ "Virtual table schema has " + types.size() + " types but " + names.size() + " names.");
+ }
+ if (rows.isEmpty()) {
+ throw new IllegalArgumentException("Virtual table must contain at least one row.");
+ }
+
+ this.types = new ArrayList<>(types);
+ this.names = new ArrayList<>(names);
+ this.rows = new ArrayList<>(rows.size());
+ for (List<Expression.Literal> row : rows) {
+ Objects.requireNonNull(row, "row");
+ if (row.size() != types.size()) {
+ throw new IllegalArgumentException(
+ "Virtual table row has "
+ + row.size()
+ + " fields but the schema has "
+ + types.size()
+ + ".");
+ }
+ ArrayList<Expression.Literal> rowCopy = new ArrayList<>(row.size());
+ for (Expression.Literal literal : row) {
+ rowCopy.add(Objects.requireNonNull(literal, "literal"));
+ }
+ this.rows.add(rowCopy);
+ }
Review Comment:
Addressed in https://github.com/apache/gluten/commit/d4dde9efb649b394588d079cd28f80d7ec5de9b4. `VirtualTableRelNode` now copies `types` and `names` element-by-element with indexed `Objects.requireNonNull` checks before relation registration. The regression cases place null at index 1, verify the indexed diagnostic, and assert that `registeredRelMap` remains empty. The focused suite passes 5/5.
##########
gluten-substrait/src/test/scala/org/apache/gluten/execution/WholeStageNoInputSuite.scala:
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gluten.execution
+
+import org.mockito.Mockito.mock
+import org.scalatest.funsuite.AnyFunSuite
+
+class WholeStageNoInputSuite extends AnyFunSuite {
+ test("explicit no-input execution creates one dependency-free partition") {
+ val wrapper = new ColumnarInputRDDsWrapper(Seq.empty, supportsNoInputExecution = true)
+
+ assert(wrapper.getPartitionLength == 1)
+ assert(wrapper.getDependencies.isEmpty)
+ assert(wrapper.getPartitions(0).isEmpty)
+ assert(wrapper.getIterators(Seq.empty, null).isEmpty)
+ }
+
+ test("an accidental empty input stage still fails") {
+ val wrapper = new ColumnarInputRDDsWrapper(Seq.empty)
+
+ val error = intercept[IllegalStateException] {
+ wrapper.getPartitionLength
+ }
+ assert(error.getMessage.contains("No non-broadcast input RDD is available"))
+ }
+
+ test("an explicitly supported broadcast-only input creates one partition") {
+ val broadcast = mock(classOf[BroadcastBuildSideRDD])
+ val wrapper =
+ new ColumnarInputRDDsWrapper(Seq(broadcast), supportsNoInputExecution = true)
+
+ assert(wrapper.getDependencies.isEmpty)
+ assert(wrapper.getPartitions(0).isEmpty)
+ assert(wrapper.getPartitionLength == 1)
+ }
+
+ test("an unsupported broadcast-only input still fails") {
+ val broadcast = mock(classOf[BroadcastBuildSideRDD])
+ val wrapper = new ColumnarInputRDDsWrapper(Seq(broadcast))
+
+ intercept[IllegalStateException] {
+ wrapper.getPartitionLength
+ }
+ }
+
+ test("concurrent partition discovery still produces one partition per request") {
+ val wrapper = new ColumnarInputRDDsWrapper(Seq.empty, supportsNoInputExecution = true)
+ val partitionLengths = Array.fill(32)(0)
+ val threads = partitionLengths.indices.map {
+ index => new Thread(() => partitionLengths(index) = wrapper.getPartitionLength)
+ }
+ threads.foreach(_.start())
+ threads.foreach(_.join())
Review Comment:
Addressed in https://github.com/apache/gluten/commit/d4dde9efb649b394588d079cd28f80d7ec5de9b4. The workers are daemon threads and now share one monotonic 10-second deadline; each join is bounded by the remaining time, followed by an assertion that no worker is still alive. The focused concurrent partition-discovery test passes.
##########
gluten-ut/spark41/src/test/scala/org/apache/spark/sql/GlutenSQLQuerySuite.scala:
##########
@@ -166,27 +168,32 @@ class GlutenSQLQuerySuite extends SQLQuerySuite with GlutenSQLTestsTrait {
=== Array(plan.stripMargin)
)
+ val oneRowPlan =
+ if (BackendTestUtils.isVeloxBackendLoaded()) {
+ """== Physical Plan ==
+ |ColumnarToRow
+ |+- ^(1) ProjectExecTransformer [1 AS 1#N]
+ | +- ^(1) OneRowRelationExecTransformer
+ |
+ |""".stripMargin
+ } else {
+ """== Physical Plan ==
+ |ColumnarToRow
+ |+- ^(1) ProjectExecTransformer [1 AS 1#N]
+ | +- ^(1) InputIteratorTransformer[]
+ | +- RowToColumnar
+ | +- *(1) Scan OneRowRelation[]
+ |
+ |""".stripMargin
+ }
Review Comment:
Addressed in https://github.com/apache/gluten/commit/d4dde9efb649b394588d079cd28f80d7ec5de9b4. The test now derives native OneRow support through `SparkShimLoader` and `BackendsApiManager`, then checks the presence or absence of the `RowToColumnar` and `Scan OneRowRelation` fallback boundaries. It no longer keys behavior to Velox or hard-codes the transformer display name. The focused Spark 4.1 explain regression passes 1/1.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "luis4a0 (via GitHub)" <gi...@apache.org>.
luis4a0 commented on code in PR #12963:
URL: https://github.com/apache/gluten/pull/12963#discussion_r4081806289
##########
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:
Addressed in https://github.com/apache/gluten/commit/ec3ed82e6d6ddb6cf852da81f1e575fda553f1e0. The test now calls the existing upstream `GlutenQueryTest.checkAnswer` with `checkToRDD = false` and removes the separate `count()`. That performs one `collect()` action while comparing the complete result, including cardinality. The corrected source compiles against the existing backend and `gluten-substrait` test dependencies.
##########
cpp/velox/tests/Substrait2VeloxValuesNodeConversionTest.cc:
##########
@@ -49,7 +53,7 @@ TEST_F(Substrait2VeloxValuesNodeConversionTest, valuesNode) {
VeloxConnectorIds{},
std::nullopt,
std::nullopt,
- true);
+ false);
Review Comment:
Addressed in https://github.com/apache/gluten/commit/ec3ed82e6d6ddb6cf852da81f1e575fda553f1e0. The non-empty-schema `valuesNode` test now constructs a fresh converter and executes `assertQuery` for both `validationMode = false` and `validationMode = true`. All 8 `Substrait2VeloxValuesNodeConversionTest` cases pass.
##########
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:
Addressed in https://github.com/apache/gluten/commit/ec3ed82e6d6ddb6cf852da81f1e575fda553f1e0. The test now uses `find()`, asserts that the entry exists and is non-null, and only then checks `TRIVIAL_LEAF`, so missing metadata produces focused assertion diagnostics. All 8 values-node conversion tests pass.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12963:
URL: https://github.com/apache/gluten/pull/12963#issuecomment-5530150750
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "luis4a0 (via GitHub)" <gi...@apache.org>.
luis4a0 commented on code in PR #12963:
URL: https://github.com/apache/gluten/pull/12963#discussion_r4072750104
##########
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:
Addressed in d74055ff8d5dafc69134910fb1cc660fcae7b17c. The converter now explicitly rejects any virtual-table row group containing fields when base_schema is absent, before cardinality derivation. Added a regression test covering both validation modes; the focused 8-test virtual-table suite and full 60-test native plan-conversion binary pass.
##########
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:
Addressed in d74055ff8d5dafc69134910fb1cc660fcae7b17c by adding the direct <unordered_map> include. The native target rebuild and clang-format-15 check both pass.
##########
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:
Addressed in d74055ff8d5dafc69134910fb1cc660fcae7b17c by changing the new builder parameter to primitive long. This API does not exist on origin/main and is introduced only by this unmerged PR, so no released boxed-signature compatibility contract exists. Direct compilation verified the J descriptor and the Scala OneRow caller.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "luis4a0 (via GitHub)" <gi...@apache.org>.
luis4a0 commented on code in PR #12963:
URL: https://github.com/apache/gluten/pull/12963#discussion_r4360674861
##########
gluten-substrait/src/main/java/org/apache/gluten/substrait/rel/VirtualTableRelNode.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gluten.substrait.rel;
+
+import org.apache.gluten.substrait.type.TypeNode;
+import org.apache.gluten.utils.SubstraitUtil;
+
+import io.substrait.proto.Expression;
+import io.substrait.proto.NamedStruct;
+import io.substrait.proto.ReadRel;
+import io.substrait.proto.Rel;
+import io.substrait.proto.RelCommon;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Objects;
+
+public class VirtualTableRelNode implements RelNode, Serializable {
+ private final List<TypeNode> types;
+ private final List<String> names;
+ private final List<List<Expression.Literal>> rows;
+
+ VirtualTableRelNode(
+ List<TypeNode> types, List<String> names, List<List<Expression.Literal>> rows) {
+ Objects.requireNonNull(types, "types");
+ Objects.requireNonNull(names, "names");
+ Objects.requireNonNull(rows, "rows");
+ if (types.size() != names.size()) {
+ throw new IllegalArgumentException(
+ "Virtual table schema has " + types.size() + " types but " + names.size() + " names.");
+ }
+ if (rows.isEmpty()) {
+ throw new IllegalArgumentException("Virtual table must contain at least one row.");
+ }
+
+ this.types = new ArrayList<>(types.size());
+ this.names = new ArrayList<>(names.size());
+ for (int index = 0; index < types.size(); ++index) {
+ this.types.add(Objects.requireNonNull(types.get(index), "types[" + index + "]"));
+ this.names.add(Objects.requireNonNull(names.get(index), "names[" + index + "]"));
+ }
+ this.rows = new ArrayList<>(rows.size());
+ for (List<Expression.Literal> row : rows) {
+ Objects.requireNonNull(row, "row");
+ if (row.size() != types.size()) {
+ throw new IllegalArgumentException(
+ "Virtual table row has "
+ + row.size()
+ + " fields but the schema has "
+ + types.size()
+ + ".");
+ }
+ ArrayList<Expression.Literal> rowCopy = new ArrayList<>(row.size());
+ for (Expression.Literal literal : row) {
+ rowCopy.add(Objects.requireNonNull(literal, "literal"));
+ }
+ this.rows.add(rowCopy);
+ }
+ }
+
+ @Override
+ public Rel toProtobuf() {
+ NamedStruct baseSchema =
+ SubstraitUtil.createNameStructBuilder(types, names, Collections.emptyList()).build();
+
+ ReadRel.VirtualTable.Builder virtualTableBuilder = ReadRel.VirtualTable.newBuilder();
+ if (types.isEmpty()) {
+ // Empty structs carry the row cardinality when there are no fields.
+ for (int row = 0; row < rows.size(); ++row) {
+ virtualTableBuilder.addExpressions(Expression.Nested.Struct.getDefaultInstance());
+ }
+ } else {
+ Expression.Nested.Struct.Builder rowGroupBuilder = Expression.Nested.Struct.newBuilder();
+ for (int column = 0; column < types.size(); ++column) {
+ for (List<Expression.Literal> row : rows) {
+ rowGroupBuilder.addFields(Expression.newBuilder().setLiteral(row.get(column)).build());
+ }
+ }
+ virtualTableBuilder.addExpressions(rowGroupBuilder.build());
Review Comment:
You're right: the non-empty-schema branch was unnecessary for this PR. I checked the callers; OneRowRelationExecTransformer is the only production caller and always supplies one empty row.
I replaced the configurable JVM serializer with OneRowRelNode and makeOneRowReadRel(context, operatorId). It now emits exactly one empty nested struct with an empty schema, with no configurable rows, literal batching or schema-copying branch. The native generic virtual-table consumer is unchanged. Focused tests cover the exact wire shape, repeatable serialization and operator registration; the existing seven OneRow integration cases pass on Spark 4.1.1 with JDK 17 and 21.
Commit: https://github.com/apache/gluten/commit/92ae892e9f4cd49d450efdc9435f9d887ec7635f
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12963:
URL: https://github.com/apache/gluten/pull/12963#issuecomment-6446479783
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12963:
URL: https://github.com/apache/gluten/pull/12963#issuecomment-5527368128
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12963:
URL: https://github.com/apache/gluten/pull/12963#issuecomment-5778914177
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "rui-mo (via GitHub)" <gi...@apache.org>.
rui-mo commented on code in PR #12963:
URL: https://github.com/apache/gluten/pull/12963#discussion_r4318900327
##########
gluten-substrait/src/main/java/org/apache/gluten/substrait/rel/VirtualTableRelNode.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gluten.substrait.rel;
+
+import org.apache.gluten.substrait.type.TypeNode;
+import org.apache.gluten.utils.SubstraitUtil;
+
+import io.substrait.proto.Expression;
+import io.substrait.proto.NamedStruct;
+import io.substrait.proto.ReadRel;
+import io.substrait.proto.Rel;
+import io.substrait.proto.RelCommon;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Objects;
+
+public class VirtualTableRelNode implements RelNode, Serializable {
+ private final List<TypeNode> types;
+ private final List<String> names;
+ private final List<List<Expression.Literal>> rows;
+
+ VirtualTableRelNode(
+ List<TypeNode> types, List<String> names, List<List<Expression.Literal>> rows) {
+ Objects.requireNonNull(types, "types");
+ Objects.requireNonNull(names, "names");
+ Objects.requireNonNull(rows, "rows");
+ if (types.size() != names.size()) {
+ throw new IllegalArgumentException(
+ "Virtual table schema has " + types.size() + " types but " + names.size() + " names.");
+ }
+ if (rows.isEmpty()) {
+ throw new IllegalArgumentException("Virtual table must contain at least one row.");
+ }
+
+ this.types = new ArrayList<>(types.size());
+ this.names = new ArrayList<>(names.size());
+ for (int index = 0; index < types.size(); ++index) {
+ this.types.add(Objects.requireNonNull(types.get(index), "types[" + index + "]"));
+ this.names.add(Objects.requireNonNull(names.get(index), "names[" + index + "]"));
+ }
+ this.rows = new ArrayList<>(rows.size());
+ for (List<Expression.Literal> row : rows) {
+ Objects.requireNonNull(row, "row");
+ if (row.size() != types.size()) {
+ throw new IllegalArgumentException(
+ "Virtual table row has "
+ + row.size()
+ + " fields but the schema has "
+ + types.size()
+ + ".");
+ }
+ ArrayList<Expression.Literal> rowCopy = new ArrayList<>(row.size());
+ for (Expression.Literal literal : row) {
+ rowCopy.add(Objects.requireNonNull(literal, "literal"));
+ }
+ this.rows.add(rowCopy);
+ }
+ }
+
+ @Override
+ public Rel toProtobuf() {
+ NamedStruct baseSchema =
+ SubstraitUtil.createNameStructBuilder(types, names, Collections.emptyList()).build();
+
+ ReadRel.VirtualTable.Builder virtualTableBuilder = ReadRel.VirtualTable.newBuilder();
+ if (types.isEmpty()) {
+ // Empty structs carry the row cardinality when there are no fields.
+ for (int row = 0; row < rows.size(); ++row) {
+ virtualTableBuilder.addExpressions(Expression.Nested.Struct.getDefaultInstance());
+ }
+ } else {
+ Expression.Nested.Struct.Builder rowGroupBuilder = Expression.Nested.Struct.newBuilder();
+ for (int column = 0; column < types.size(); ++column) {
+ for (List<Expression.Literal> row : rows) {
+ rowGroupBuilder.addFields(Expression.newBuilder().setLiteral(row.get(column)).build());
+ }
+ }
+ virtualTableBuilder.addExpressions(rowGroupBuilder.build());
Review Comment:
Could you clarify why this branch is needed? My understanding is that Spark's OneRowRelationExec always supplies one empty row.
https://github.com/apache/spark/blame/master/sql/core/src/main/scala/org/apache/spark/sql/execution/ExistingRDD.scala#L359
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "luis4a0 (via GitHub)" <gi...@apache.org>.
luis4a0 commented on PR #12963:
URL: https://github.com/apache/gluten/pull/12963#issuecomment-6446071389
CI follow-up pushed in https://github.com/apache/gluten/commit/00a98ccffa40e21bdf7a4caa96b476c7f5b3704e .
The Spark 4.2 group-3 job reported two expected-plan mismatches, for TPC-DS v2.7 `q77a` and `q77a.sf100`:
https://github.com/apache/gluten/actions/runs/40514673099/job/122230938416
The actual plans correctly used `OneRowRelationExecTransformer`, but those two golden plans still expected Spark's OneRow scan and its row/column adapters. I regenerated both simplified and formatted outputs with the existing golden-file generator, including the corresponding formatted-plan operator references. This changes only four test-resource files: no runtime code, query text, test selectors, or assertions changed. The Spark 4.1 counterpart plans do not contain this OneRow subtree and were left unchanged.
Both exact cases failed locally before the update. With golden generation disabled afterward, both complete TPC-DS v2.7 plan-stability suites plus the OneRow execution and parameterized-EXPLAIN regressions passed: 66 tests, zero failures or ignored tests.
The new head still needs its own fresh CI and review; this does not relabel the previous failed run as passing.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "luis4a0 (via GitHub)" <gi...@apache.org>.
luis4a0 commented on code in PR #12963:
URL: https://github.com/apache/gluten/pull/12963#discussion_r4102739663
##########
cpp/velox/tests/Substrait2VeloxValuesNodeConversionTest.cc:
##########
@@ -62,7 +56,250 @@ TEST_F(Substrait2VeloxValuesNodeConversionTest, valuesNode) {
});
createDuckDbTable({expectedData});
- assertQuery(veloxPlan, "SELECT * FROM tmp");
+ for (const bool validationMode : {false, true}) {
+ 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,
+ validationMode);
+ assertQuery(planConverter->toVeloxPlan(substraitPlan), "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);
+ const auto& splitInfos = planConverter->splitInfos();
+ const auto splitInfo = splitInfos.find(valuesNode->id());
+ ASSERT_NE(splitInfo, splitInfos.end());
+ ASSERT_NE(splitInfo->second, nullptr);
+ ASSERT_EQ(splitInfo->second->leafType, SplitInfo::LeafType::TRIVIAL_LEAF);
+
+ CursorParameters params;
+ params.planNode = veloxPlan;
+ auto [cursor, results] = readCursor(params);
Review Comment:
Addressed in https://github.com/apache/gluten/commit/5cf111fb8dfff46a141f980b9db5f7c9697ec27f. The test now keeps the `readCursor` return value in one named pair and reads `.second`, so no unused structured-binding variable is introduced while the cursor remains alive through the assertions. The focused native target builds and all 8 values-node conversion tests pass.
##########
cpp/velox/tests/Substrait2VeloxValuesNodeConversionTest.cc:
##########
@@ -62,7 +56,250 @@ TEST_F(Substrait2VeloxValuesNodeConversionTest, valuesNode) {
});
createDuckDbTable({expectedData});
- assertQuery(veloxPlan, "SELECT * FROM tmp");
+ for (const bool validationMode : {false, true}) {
+ 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,
+ validationMode);
+ assertQuery(planConverter->toVeloxPlan(substraitPlan), "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);
+ const auto& splitInfos = planConverter->splitInfos();
+ const auto splitInfo = splitInfos.find(valuesNode->id());
+ ASSERT_NE(splitInfo, splitInfos.end());
+ ASSERT_NE(splitInfo->second, nullptr);
+ ASSERT_EQ(splitInfo->second->leafType, SplitInfo::LeafType::TRIVIAL_LEAF);
+
+ CursorParameters params;
+ params.planNode = veloxPlan;
+ auto [cursor, results] = readCursor(params);
+ ASSERT_EQ(results.size(), 1);
+ ASSERT_EQ(results.front()->childrenSize(), 0);
+ ASSERT_EQ(results.front()->size(), 1);
+}
+
+TEST_F(Substrait2VeloxValuesNodeConversionTest, virtualTableDoesNotConsumeTableScanSplit) {
+ auto planPath = FilePathGenerator::getDataFilePath("substrait_virtualTable.json");
+
+ ::substrait::Plan substraitPlan;
+ JsonToProtoConverter::readFromFile(planPath, substraitPlan);
+ const auto virtualRead = substraitPlan.relations(0).root().input().read();
+
+ auto* setRel = substraitPlan.mutable_relations(0)->mutable_root()->mutable_input()->mutable_set();
+ setRel->set_op(::substrait::SetRel_SetOp::SetRel_SetOp_SET_OP_UNION_ALL);
+ setRel->add_inputs()->mutable_read()->CopyFrom(virtualRead);
+ auto* tableScanRead = setRel->add_inputs()->mutable_read();
+ tableScanRead->CopyFrom(virtualRead);
+ tableScanRead->clear_virtual_table();
+
+ auto veloxCfg = std::make_shared<facebook::velox::config::ConfigBase>(std::unordered_map<std::string, std::string>());
+ VeloxConnectorIds connectorIds;
+ connectorIds.hive = "test-hive";
+ auto planConverter = std::make_shared<SubstraitToVeloxPlanConverter>(
+ pool_.get(),
+ veloxCfg.get(),
+ std::vector<std::shared_ptr<ResultIterator>>{},
+ std::move(connectorIds),
+ std::nullopt,
+ std::nullopt,
+ false);
+ auto tableScanSplit = std::make_shared<SplitInfo>();
+ tableScanSplit->leafType = SplitInfo::LeafType::TABLE_SCAN;
+ planConverter->setSplitInfos({tableScanSplit});
+
+ auto veloxPlan = planConverter->toVeloxPlan(substraitPlan);
+ ASSERT_NE(std::dynamic_pointer_cast<const core::LocalPartitionNode>(veloxPlan), nullptr);
+
+ const auto& splitInfos = planConverter->splitInfos();
+ ASSERT_EQ(splitInfos.size(), 2);
+ ASSERT_EQ(
+ std::count_if(
+ splitInfos.begin(),
+ splitInfos.end(),
+ [](const auto& entry) { return entry.second->leafType == SplitInfo::LeafType::TRIVIAL_LEAF; }),
+ 1);
+ ASSERT_EQ(
+ std::count_if(
+ splitInfos.begin(), splitInfos.end(), [&](const auto& entry) { return entry.second == tableScanSplit; }),
+ 1);
+}
+
+TEST_F(Substrait2VeloxValuesNodeConversionTest, rejectsEmptyVirtualTableInAllModes) {
+ auto planPath = FilePathGenerator::getDataFilePath("substrait_virtualTable_emptySchema.json");
+ for (const bool validationMode : {false, true}) {
+ ::substrait::Plan substraitPlan;
+ JsonToProtoConverter::readFromFile(planPath, substraitPlan);
+ substraitPlan.mutable_relations(0)
+ ->mutable_root()
+ ->mutable_input()
+ ->mutable_read()
+ ->mutable_virtual_table()
+ ->clear_expressions();
+
+ 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,
+ validationMode);
+
+ VELOX_ASSERT_THROW(planConverter->toVeloxPlan(substraitPlan), "Virtual table must contain at least one row group.");
+ }
+}
+
+TEST_F(Substrait2VeloxValuesNodeConversionTest, supportsZeroRowVirtualTableInAllModes) {
+ auto planPath = FilePathGenerator::getDataFilePath("substrait_virtualTable.json");
+ for (const bool validationMode : {false, true}) {
+ ::substrait::Plan substraitPlan;
+ JsonToProtoConverter::readFromFile(planPath, substraitPlan);
+ substraitPlan.mutable_relations(0)
+ ->mutable_root()
+ ->mutable_input()
+ ->mutable_read()
+ ->mutable_virtual_table()
+ ->mutable_expressions(0)
+ ->clear_fields();
+
+ 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,
+ validationMode);
+
+ auto values =
+ std::dynamic_pointer_cast<const facebook::velox::core::ValuesNode>(planConverter->toVeloxPlan(substraitPlan));
+ ASSERT_NE(values, nullptr);
+ ASSERT_EQ(values->values().size(), 1);
+ ASSERT_EQ(values->values().front()->size(), 0);
+ if (!validationMode) {
+ CursorParameters params;
+ params.planNode = values;
+ auto [cursor, results] = readCursor(params);
Review Comment:
Addressed in https://github.com/apache/gluten/commit/5cf111fb8dfff46a141f980b9db5f7c9697ec27f with the same single-pair pattern. The test reads results through `.second` without creating an unused `cursor` binding. The focused native target builds and all 8 values-node conversion tests pass.
##########
gluten-substrait/src/test/scala/org/apache/gluten/execution/WholeStageNoInputSuite.scala:
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gluten.execution
+
+import org.mockito.Mockito.mock
+import org.scalatest.funsuite.AnyFunSuite
+
+import java.util.concurrent.TimeUnit
+
+class WholeStageNoInputSuite extends AnyFunSuite {
+ test("explicit no-input execution creates one dependency-free partition") {
+ val wrapper = new ColumnarInputRDDsWrapper(Seq.empty, supportsNoInputExecution = true)
+
+ assert(wrapper.getPartitionLength == 1)
+ assert(wrapper.getDependencies.isEmpty)
+ assert(wrapper.getPartitions(0).isEmpty)
+ assert(wrapper.getIterators(Seq.empty, null).isEmpty)
Review Comment:
Addressed in https://github.com/apache/gluten/commit/5cf111fb8dfff46a141f980b9db5f7c9697ec27f. The test now passes a Mockito `TaskContext` test double instead of `null`, using the suite’s existing Mockito dependency. The exact final `WholeStageNoInputSuite` source compiles and all 5 tests pass.
##########
shims/common/src/main/scala/org/apache/gluten/sql/shims/SparkShims.scala:
##########
@@ -173,6 +173,8 @@ trait SparkShims {
*/
def getLocalTableScanStream(plan: LocalTableScanExec): Option[SparkDataStream] = None
Review Comment:
Addressed in https://github.com/apache/gluten/commit/5cf111fb8dfff46a141f980b9db5f7c9697ec27f. The Scaladoc now states that this is exact recognition of Spark 4.1+ `OneRowRelationExec`, explains why earlier shims return `false`, and warns callers not to infer generic zero-input semantics from `false`.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12963:
URL: https://github.com/apache/gluten/pull/12963#issuecomment-5731316136
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12963:
URL: https://github.com/apache/gluten/pull/12963#discussion_r4072939818
##########
gluten-substrait/src/main/java/org/apache/gluten/substrait/rel/VirtualTableRelNode.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gluten.substrait.rel;
+
+import org.apache.gluten.substrait.type.TypeNode;
+import org.apache.gluten.utils.SubstraitUtil;
+
+import io.substrait.proto.Expression;
+import io.substrait.proto.NamedStruct;
+import io.substrait.proto.ReadRel;
+import io.substrait.proto.Rel;
+import io.substrait.proto.RelCommon;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Objects;
+
+public class VirtualTableRelNode implements RelNode, Serializable {
+ private final List<TypeNode> types;
+ private final List<String> names;
+ private final List<List<Expression.Literal>> rows;
+
+ VirtualTableRelNode(
+ List<TypeNode> types, List<String> names, List<List<Expression.Literal>> rows) {
+ Objects.requireNonNull(types, "types");
+ Objects.requireNonNull(names, "names");
+ Objects.requireNonNull(rows, "rows");
+ if (types.size() != names.size()) {
+ throw new IllegalArgumentException(
+ "Virtual table schema has " + types.size() + " types but " + names.size() + " names.");
+ }
+ if (rows.isEmpty()) {
+ throw new IllegalArgumentException("Virtual table must contain at least one row.");
+ }
+
+ this.types = new ArrayList<>(types);
+ this.names = new ArrayList<>(names);
+ this.rows = new ArrayList<>(rows.size());
+ for (List<Expression.Literal> row : rows) {
+ Objects.requireNonNull(row, "row");
+ if (row.size() != types.size()) {
+ throw new IllegalArgumentException(
+ "Virtual table row has "
+ + row.size()
+ + " fields but the schema has "
+ + types.size()
+ + ".");
+ }
+ ArrayList<Expression.Literal> rowCopy = new ArrayList<>(row.size());
+ for (Expression.Literal literal : row) {
+ rowCopy.add(Objects.requireNonNull(literal, "literal"));
+ }
+ this.rows.add(rowCopy);
+ }
Review Comment:
The constructor validates null `rows`/`row`/`literal`, but does not validate that individual `types` and `names` elements are non-null. If a caller passes a list containing null elements, the object can be constructed and registered, but later `toProtobuf()` may fail with a less actionable NPE (and after registration has occurred). Consider validating each `TypeNode` and each `String` name with `Objects.requireNonNull` (optionally including the index in the message) before copying.
##########
gluten-substrait/src/test/scala/org/apache/gluten/execution/WholeStageNoInputSuite.scala:
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gluten.execution
+
+import org.mockito.Mockito.mock
+import org.scalatest.funsuite.AnyFunSuite
+
+class WholeStageNoInputSuite extends AnyFunSuite {
+ test("explicit no-input execution creates one dependency-free partition") {
+ val wrapper = new ColumnarInputRDDsWrapper(Seq.empty, supportsNoInputExecution = true)
+
+ assert(wrapper.getPartitionLength == 1)
+ assert(wrapper.getDependencies.isEmpty)
+ assert(wrapper.getPartitions(0).isEmpty)
+ assert(wrapper.getIterators(Seq.empty, null).isEmpty)
+ }
+
+ test("an accidental empty input stage still fails") {
+ val wrapper = new ColumnarInputRDDsWrapper(Seq.empty)
+
+ val error = intercept[IllegalStateException] {
+ wrapper.getPartitionLength
+ }
+ assert(error.getMessage.contains("No non-broadcast input RDD is available"))
+ }
+
+ test("an explicitly supported broadcast-only input creates one partition") {
+ val broadcast = mock(classOf[BroadcastBuildSideRDD])
+ val wrapper =
+ new ColumnarInputRDDsWrapper(Seq(broadcast), supportsNoInputExecution = true)
+
+ assert(wrapper.getDependencies.isEmpty)
+ assert(wrapper.getPartitions(0).isEmpty)
+ assert(wrapper.getPartitionLength == 1)
+ }
+
+ test("an unsupported broadcast-only input still fails") {
+ val broadcast = mock(classOf[BroadcastBuildSideRDD])
+ val wrapper = new ColumnarInputRDDsWrapper(Seq(broadcast))
+
+ intercept[IllegalStateException] {
+ wrapper.getPartitionLength
+ }
+ }
+
+ test("concurrent partition discovery still produces one partition per request") {
+ val wrapper = new ColumnarInputRDDsWrapper(Seq.empty, supportsNoInputExecution = true)
+ val partitionLengths = Array.fill(32)(0)
+ val threads = partitionLengths.indices.map {
+ index => new Thread(() => partitionLengths(index) = wrapper.getPartitionLength)
+ }
+ threads.foreach(_.start())
+ threads.foreach(_.join())
Review Comment:
The test uses `Thread.join()` without a timeout. If `getPartitionLength` ever regresses into a deadlock or infinite wait, the suite can hang indefinitely. Consider joining with a bounded timeout and asserting all threads completed (e.g., `threads.foreach(_.join(…))` plus an `assert(!thread.isAlive)`), so failures are reported as test failures rather than stalled runs.
##########
gluten-ut/spark41/src/test/scala/org/apache/spark/sql/GlutenSQLQuerySuite.scala:
##########
@@ -166,27 +168,32 @@ class GlutenSQLQuerySuite extends SQLQuerySuite with GlutenSQLTestsTrait {
=== Array(plan.stripMargin)
)
+ val oneRowPlan =
+ if (BackendTestUtils.isVeloxBackendLoaded()) {
+ """== Physical Plan ==
+ |ColumnarToRow
+ |+- ^(1) ProjectExecTransformer [1 AS 1#N]
+ | +- ^(1) OneRowRelationExecTransformer
+ |
+ |""".stripMargin
+ } else {
+ """== Physical Plan ==
+ |ColumnarToRow
+ |+- ^(1) ProjectExecTransformer [1 AS 1#N]
+ | +- ^(1) InputIteratorTransformer[]
+ | +- RowToColumnar
+ | +- *(1) Scan OneRowRelation[]
+ |
+ |""".stripMargin
+ }
Review Comment:
This regression expectation is keyed specifically to `isVeloxBackendLoaded()`. If another backend later adds native `OneRowRelationExec` support (or Velox changes the printed node name), this test will fail despite correct behavior. A more robust approach is to assert on the presence/absence of the specific fallback boundary lines (e.g., `RowToColumnar` + `Scan OneRowRelation`) rather than hard-coding an entire backend-specific plan string, or to key the expectation off a capability signal (e.g., whether `genOneRowRelationExecTransformer` is overridden/active) instead of the backend name.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12963:
URL: https://github.com/apache/gluten/pull/12963#issuecomment-5777964999
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Support native Spark 4.1 OneRowRelationExec [gluten]
Posted by "luis4a0 (via GitHub)" <gi...@apache.org>.
luis4a0 commented on PR #12963:
URL: https://github.com/apache/gluten/pull/12963#issuecomment-6443469978
Refreshed this branch by merging upstream main c5bd3de4abf78942c3691deb97f79962e81072ed. The resolutions preserve both early virtual-table conversion and main's Kafka handling, plus both required operator-suite imports.
Main introduced a separate Spark 4.2 shim, so I also wired exact OneRowRelationExec recognition there and mirrored the existing native-aware EXPLAIN/settings integration. The new recognition regression failed before that override and passes afterward.
Fresh local validation passed:
- 17 native plan-conversion/validation tests and 15 JVM serializer/no-input/partition tests.
- All 106 executed MiscOperatorSuite cases on Spark 4.1.1 (one existing ignored test), plus the seven OneRow cases on JDK 21.
- The Spark 4.1 EXPLAIN case, three Spark 4.2 shim cases, and two executed Spark 4.2 SQL cases covering actual native values/plans/partitioning across AQE/codegen modes and EXPLAIN.
- Explicit formatting/style checks and matched rebuilt native-library packaging.
Main's unrelated MiscOperatorSuite quarantine on Spark 4.2 is unchanged. Its ignored invocation is not counted as validation; the added versioned SQL test provides actual execution coverage. Fresh merge CI is still required.
Refresh commit: https://github.com/apache/gluten/commit/876c798b9914501eadddcdc151b09338a3bbfd6a
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org