This is an automated email from the ASF dual-hosted git repository.
zhouyuan pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gluten.git
The following commit(s) were added to refs/heads/main by this push:
new 92581ca8e3 [GLUTEN-12597][CORE] Migrate ReadRel.VirtualTable to
Substrait 0.98 (values -> expressions) (#12849)
92581ca8e3 is described below
commit 92581ca8e3d754d887cbfacdc0ff372cd81d80cc
Author: Niels Pardon <[email protected]>
AuthorDate: Wed Aug 26 10:35:03 2026 +0200
[GLUTEN-12597][CORE] Migrate ReadRel.VirtualTable to Substrait 0.98 (values
-> expressions) (#12849)
---
cpp/velox/substrait/SubstraitToVeloxPlan.cc | 34 +++---
cpp/velox/substrait/VeloxToSubstraitExpr.cc | 8 +-
cpp/velox/substrait/VeloxToSubstraitExpr.h | 10 +-
cpp/velox/substrait/VeloxToSubstraitPlan.cc | 18 ++-
cpp/velox/tests/data/substrait_virtualTable.json | 122 +++++++++++++--------
.../substrait/proto/substrait/algebra.proto | 7 +-
.../substrait/rel/VirtualTableProtoSuite.scala | 85 ++++++++++++++
7 files changed, 206 insertions(+), 78 deletions(-)
diff --git a/cpp/velox/substrait/SubstraitToVeloxPlan.cc
b/cpp/velox/substrait/SubstraitToVeloxPlan.cc
index 21320519b0..7fbfec5b85 100644
--- a/cpp/velox/substrait/SubstraitToVeloxPlan.cc
+++ b/cpp/velox/substrait/SubstraitToVeloxPlan.cc
@@ -1688,26 +1688,23 @@ core::PlanNodePtr
SubstraitToVeloxPlanConverter::toVeloxPlan(const ::substrait::
core::PlanNodePtr SubstraitToVeloxPlanConverter::toVeloxPlan(
const ::substrait::ReadRel& readRel,
const RowTypePtr& type) {
- ::substrait::ReadRel_VirtualTable readVirtualTable = readRel.virtual_table();
- int64_t numVectors = readVirtualTable.values_size();
- int64_t numColumns = type->size();
- int64_t valueFieldNums = readVirtualTable.values(numVectors -
1).fields_size();
+ const ::substrait::ReadRel_VirtualTable& readVirtualTable =
readRel.virtual_table();
+ const int64_t numVectors = readVirtualTable.expressions_size();
+ const int64_t numColumns = type->size();
std::vector<RowVectorPtr> vectors;
vectors.reserve(numVectors);
- int64_t batchSize;
- // For the empty vectors, eg,vectors = makeRowVector(ROW({}, {}), 1).
- if (numColumns == 0) {
- batchSize = 1;
- } else {
- batchSize = valueFieldNums / numColumns;
- }
-
for (int64_t index = 0; index < numVectors; ++index) {
std::vector<VectorPtr> children;
- ::substrait::Expression_Literal_Struct rowValue =
readRel.virtual_table().values(index);
- auto fieldSize = rowValue.fields_size();
- VELOX_CHECK_EQ(fieldSize, batchSize * numColumns);
+ // Each Nested.Struct holds one row group, laid out column-major. Row
groups need not all
+ // carry the same number of rows, so derive the batch size per struct
rather than once for
+ // the whole table.
+ const ::substrait::Expression_Nested_Struct& rowValue =
readVirtualTable.expressions(index);
+ const int64_t fieldSize = rowValue.fields_size();
+ // For the empty vectors, eg,vectors = makeRowVector(ROW({}, {}), 1).
+ const int64_t batchSize = numColumns == 0 ? 1 : fieldSize / numColumns;
+ VELOX_USER_CHECK_EQ(
+ fieldSize, batchSize * numColumns, "ReadRel.VirtualTable field count
must be a multiple of the column count.");
for (int64_t col = 0; col < numColumns; ++col) {
const TypePtr& outputChildType = type->childAt(col);
@@ -1716,7 +1713,12 @@ core::PlanNodePtr
SubstraitToVeloxPlanConverter::toVeloxPlan(
for (int64_t batchId = 0; batchId < batchSize; batchId++) {
// each value in the batch
auto fieldIdx = col * batchSize + batchId;
- ::substrait::Expression_Literal field = rowValue.fields(fieldIdx);
+ // Substrait models virtual table values as Expressions; Gluten only
ever emits literals,
+ // so unwrap back to the Literal the downstream conversion expects.
This converter is also
+ // reachable from the JSON-plan test and benchmark paths, so reject
anything else loudly.
+ const ::substrait::Expression& fieldExpr = rowValue.fields(fieldIdx);
+ VELOX_USER_CHECK(fieldExpr.has_literal(), "ReadRel.VirtualTable
expressions must be literals.");
+ const ::substrait::Expression_Literal& field = fieldExpr.literal();
auto expr = exprConverter_->toVeloxExpr(field);
if (auto constantExpr = std::dynamic_pointer_cast<const
core::ConstantTypedExpr>(expr)) {
diff --git a/cpp/velox/substrait/VeloxToSubstraitExpr.cc
b/cpp/velox/substrait/VeloxToSubstraitExpr.cc
index 807c342cbd..f9cd1d2880 100644
--- a/cpp/velox/substrait/VeloxToSubstraitExpr.cc
+++ b/cpp/velox/substrait/VeloxToSubstraitExpr.cc
@@ -292,7 +292,7 @@ template <TypeKind kind>
void convertVectorValue(
google::protobuf::Arena& arena,
const velox::VectorPtr& vectorValue,
- ::substrait::Expression_Literal_Struct* litValue,
+ ::substrait::Expression_Nested_Struct* litValue,
::substrait::Expression_Literal* substraitField) {
const TypePtr& childType = vectorValue->type();
@@ -303,7 +303,7 @@ void convertVectorValue(
// Get the batchSize and convert each value in it.
vector_size_t flatVecSize = childToFlatVec->size();
for (int64_t i = 0; i < flatVecSize; i++) {
- substraitField = litValue->add_fields();
+ substraitField = litValue->add_fields()->mutable_literal();
if (childToFlatVec->isNullAt(i)) {
// Process the null value.
substraitField->MergeFrom(toSubstraitNullLiteral(arena,
childType->kind()));
@@ -512,7 +512,7 @@ const ::substrait::Expression&
VeloxToSubstraitExprConvertor::toSubstraitExpr(
const ::substrait::Expression_Literal&
VeloxToSubstraitExprConvertor::toSubstraitExpr(
google::protobuf::Arena& arena,
const std::shared_ptr<const core::ConstantTypedExpr>& constExpr,
- ::substrait::Expression_Literal_Struct* litValue) {
+ ::substrait::Expression_Nested_Struct* litValue) {
if (constExpr->hasValueVector()) {
return toSubstraitLiteral(arena, constExpr->valueVector(), litValue);
} else {
@@ -595,7 +595,7 @@ const ::substrait::Expression_Literal&
VeloxToSubstraitExprConvertor::toSubstrai
const ::substrait::Expression_Literal&
VeloxToSubstraitExprConvertor::toSubstraitLiteral(
google::protobuf::Arena& arena,
const velox::VectorPtr& vectorValue,
- ::substrait::Expression_Literal_Struct* litValue) {
+ ::substrait::Expression_Nested_Struct* litValue) {
::substrait::Expression_Literal* substraitField =
google::protobuf::Arena::CreateMessage<::substrait::Expression_Literal>(&arena);
if (vectorValue->isScalar()) {
diff --git a/cpp/velox/substrait/VeloxToSubstraitExpr.h
b/cpp/velox/substrait/VeloxToSubstraitExpr.h
index c6f65ddf45..83b9a5260f 100644
--- a/cpp/velox/substrait/VeloxToSubstraitExpr.h
+++ b/cpp/velox/substrait/VeloxToSubstraitExpr.h
@@ -45,13 +45,14 @@ class VeloxToSubstraitExprConvertor {
/// Literal Expression.
/// @param arena Arena to use for allocating Substrait plan objects.
/// @param constExpr Velox Constant expression needed to be converted.
- /// @param litValue The Struct that returned literal expression belong to.
+ /// @param litValue The Nested.Struct the converted literals are appended
to, each wrapped in an
+ /// Expression. Substrait models struct-valued expressions as
Expression.Nested.Struct.
/// @return A pointer to Substrait Literal expression object allocated on
/// the arena and representing the input Velox Constant expression.
const ::substrait::Expression_Literal& toSubstraitExpr(
google::protobuf::Arena& arena,
const std::shared_ptr<const core::ConstantTypedExpr>& constExpr,
- ::substrait::Expression_Literal_Struct* litValue = nullptr);
+ ::substrait::Expression_Nested_Struct* litValue = nullptr);
/// Convert Velox FieldAccessTypedExpr to Substrait FieldReference
Expression.
const ::substrait::Expression_FieldReference& toSubstraitExpr(
@@ -64,11 +65,12 @@ class VeloxToSubstraitExprConvertor {
const std::shared_ptr<const core::DereferenceTypedExpr>& derefExpr,
const RowTypePtr& inputType);
- /// Convert Velox vector to Substrait literal.
+ /// Convert Velox vector to Substrait literal. One literal per row is
appended to litValue,
+ /// each wrapped in an Expression so it fits Substrait's
Expression.Nested.Struct container.
const ::substrait::Expression_Literal& toSubstraitLiteral(
google::protobuf::Arena& arena,
const velox::VectorPtr& vectorValue,
- ::substrait::Expression_Literal_Struct* litValue);
+ ::substrait::Expression_Nested_Struct* litValue);
private:
/// Convert Velox Cast Expression to Substrait Cast Expression.
diff --git a/cpp/velox/substrait/VeloxToSubstraitPlan.cc
b/cpp/velox/substrait/VeloxToSubstraitPlan.cc
index cdbea2ef32..3b0c6d6b4f 100644
--- a/cpp/velox/substrait/VeloxToSubstraitPlan.cc
+++ b/cpp/velox/substrait/VeloxToSubstraitPlan.cc
@@ -187,13 +187,19 @@ void VeloxToSubstraitPlanConvertor::toSubstrait(
::substrait::ReadRel_VirtualTable* virtualTable =
readRel->mutable_virtual_table();
for (const auto& vector : valuesNode->values()) {
- ::substrait::Expression_Literal_Struct* litValue =
virtualTable->add_values();
-
+ // Substrait models a virtual table row group as an
Expression.Nested.Struct laid out
+ // column-major; toSubstraitLiteral appends one Expression per value into
it.
+ ::substrait::Expression_Nested_Struct* nested =
virtualTable->add_expressions();
for (const auto& column : vector->children()) {
- ::substrait::Expression_Literal* substraitField =
-
google::protobuf::Arena::CreateMessage<::substrait::Expression_Literal>(&arena);
-
- substraitField->MergeFrom(exprConvertor_->toSubstraitLiteral(arena,
column, litValue));
+ const int expectedFields = nested->fields_size() + vector->size();
+ exprConvertor_->toSubstraitLiteral(arena, column, nested);
+ // Only scalar columns are appended; complex-typed ones are returned by
value instead, which
+ // would silently shorten the row group and transpose the table the
consumer decodes from it.
+ VELOX_USER_CHECK_EQ(
+ nested->fields_size(),
+ expectedFields,
+ "Unsupported virtual table column type: {}",
+ column->type()->toString());
}
}
diff --git a/cpp/velox/tests/data/substrait_virtualTable.json
b/cpp/velox/tests/data/substrait_virtualTable.json
index 5888df257e..99505c8ae9 100644
--- a/cpp/velox/tests/data/substrait_virtualTable.json
+++ b/cpp/velox/tests/data/substrait_virtualTable.json
@@ -55,83 +55,113 @@
}
},
"virtual_table": {
- "values": [
+ "expressions": [
{
"fields": [
{
- "nullable": false,
- "i64": "2499109626526694126"
+ "literal": {
+ "nullable": false,
+ "i64": "2499109626526694126"
+ }
},
{
- "nullable": false,
- "i64": "2342493223442167775"
+ "literal": {
+ "nullable": false,
+ "i64": "2342493223442167775"
+ }
},
{
- "nullable": false,
- "i64": "4077358421272316858"
+ "literal": {
+ "nullable": false,
+ "i64": "4077358421272316858"
+ }
},
{
- "nullable": false,
- "i32": 581869302
+ "literal": {
+ "nullable": false,
+ "i32": 581869302
+ }
},
{
- "nullable": false,
- "i32": -708632711
+ "literal": {
+ "nullable": false,
+ "i32": -708632711
+ }
},
{
- "nullable": false,
- "i32": -133711905
+ "literal": {
+ "nullable": false,
+ "i32": -133711905
+ }
},
{
- "nullable": false,
- "fp64": 0.90579193414549275
+ "literal": {
+ "nullable": false,
+ "fp64": 0.90579193414549275
+ }
},
{
- "nullable": false,
- "fp64": 0.96886777112423139
+ "literal": {
+ "nullable": false,
+ "fp64": 0.96886777112423139
+ }
},
{
- "nullable": false,
- "fp64": 0.63235925003444637
+ "literal": {
+ "nullable": false,
+ "fp64": 0.63235925003444637
+ }
},
{
- "nullable": false,
- "boolean": true
+ "literal": {
+ "nullable": false,
+ "boolean": true
+ }
},
{
- "nullable": false,
- "boolean": false
+ "literal": {
+ "nullable": false,
+ "boolean": false
+ }
},
{
- "nullable": false,
- "boolean": false
+ "literal": {
+ "nullable": false,
+ "boolean": false
+ }
},
{
- "null": {
- "i32": {
- "type_variation_reference": 0,
- "nullability": "NULLABILITY_NULLABLE"
- }
- },
- "nullable": true
+ "literal": {
+ "null": {
+ "i32": {
+ "type_variation_reference": 0,
+ "nullability": "NULLABILITY_NULLABLE"
+ }
+ },
+ "nullable": true
+ }
},
{
- "null": {
- "i32": {
- "type_variation_reference": 0,
- "nullability": "NULLABILITY_NULLABLE"
- }
- },
- "nullable": true
+ "literal": {
+ "null": {
+ "i32": {
+ "type_variation_reference": 0,
+ "nullability": "NULLABILITY_NULLABLE"
+ }
+ },
+ "nullable": true
+ }
},
{
- "null": {
- "i32": {
- "type_variation_reference": 0,
- "nullability": "NULLABILITY_NULLABLE"
- }
- },
- "nullable": true
+ "literal": {
+ "null": {
+ "i32": {
+ "type_variation_reference": 0,
+ "nullability": "NULLABILITY_NULLABLE"
+ }
+ },
+ "nullable": true
+ }
}
]
}
diff --git
a/gluten-substrait/src/main/resources/substrait/proto/substrait/algebra.proto
b/gluten-substrait/src/main/resources/substrait/proto/substrait/algebra.proto
index e239691208..ad74b2b86d 100644
---
a/gluten-substrait/src/main/resources/substrait/proto/substrait/algebra.proto
+++
b/gluten-substrait/src/main/resources/substrait/proto/substrait/algebra.proto
@@ -79,9 +79,12 @@ message ReadRel {
substrait.extensions.AdvancedExtension advanced_extension = 10;
}
- // A table composed of literals.
+ // A table composed of expressions.
message VirtualTable {
- repeated Expression.Literal.Struct values = 1;
+ reserved 1;
+ reserved "values";
+
+ repeated Expression.Nested.Struct expressions = 2;
}
// A stub type that can be used to extend/introduce new table types outside
diff --git
a/gluten-substrait/src/test/scala/org/apache/gluten/substrait/rel/VirtualTableProtoSuite.scala
b/gluten-substrait/src/test/scala/org/apache/gluten/substrait/rel/VirtualTableProtoSuite.scala
new file mode 100644
index 0000000000..4dd8d2de53
--- /dev/null
+++
b/gluten-substrait/src/test/scala/org/apache/gluten/substrait/rel/VirtualTableProtoSuite.scala
@@ -0,0 +1,85 @@
+/*
+ * 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 com.google.protobuf.Descriptors.{Descriptor, FieldDescriptor}
+import io.substrait.proto.{Expression, ReadRel}
+import org.scalatest.funsuite.AnyFunSuite
+
+/**
+ * Pins the wire tags of the vendored `ReadRel.VirtualTable` after its rebase
onto upstream
+ * Substrait v0.98.0. A round trip through the generated classes cannot catch
a renumber or a field
+ * rename, because producer and consumer share one schema, so these assert on
the descriptors
+ * instead. There is no JVM producer for virtual tables - the only producer
and consumer live in
+ * cpp/velox/substrait - which is precisely why nothing else in the build
would notice a regression
+ * here.
+ */
+class VirtualTableProtoSuite extends AnyFunSuite {
+
+ private def field(name: String, descriptor: Descriptor): FieldDescriptor = {
+ val f = descriptor.findFieldByName(name)
+ assert(f != null, s"${descriptor.getName} has no field named $name")
+ f
+ }
+
+ test("VirtualTable carries expressions on tag 2 and no longer declares
values") {
+ val descriptor = ReadRel.VirtualTable.getDescriptor
+
+ val expressions = field("expressions", descriptor)
+ assert(expressions.getNumber === 2, "VirtualTable.expressions changed its
number")
+ assert(expressions.isRepeated, "VirtualTable.expressions must stay
repeated")
+ assert(
+ expressions.getMessageType.getFullName ===
"substrait.Expression.Nested.Struct",
+ "VirtualTable.expressions must hold Expression.Nested.Struct, not
Expression.Literal.Struct"
+ )
+
+ // Upstream reserved tag 1 and the `values` name; a rebase must not
reintroduce either.
+ assert(
+ descriptor.findFieldByName("values") === null,
+ "the pre-0.98 VirtualTable.values field must be gone")
+ assert(
+ descriptor.findFieldByNumber(1) === null,
+ "VirtualTable tag 1 is reserved upstream and must stay unused")
+ }
+
+ test("virtual_table stays on read_type tag 5") {
+ val virtualTable = field("virtual_table", ReadRel.getDescriptor)
+ assert(virtualTable.getNumber === 5, "ReadRel.virtual_table changed its
number")
+ assert(
+ virtualTable.getContainingOneof != null &&
+ virtualTable.getContainingOneof.getName === "read_type",
+ "ReadRel.virtual_table must stay in the read_type oneof"
+ )
+ }
+
+ test("Nested.Struct fields are Expressions, so each value needs a literal
wrapper") {
+ val fields = field("fields", Expression.Nested.Struct.getDescriptor)
+ assert(fields.getNumber === 1)
+ assert(fields.isRepeated)
+ assert(
+ fields.getMessageType.getFullName === "substrait.Expression",
+ "Nested.Struct.fields must hold Expression - this is what forces the
.literal() unwrap in " +
+ "SubstraitToVeloxPlan"
+ )
+
+ // The literal wrapper the C++ producer and consumer rely on.
+ val literal = field("literal", Expression.getDescriptor)
+ assert(literal.getNumber === 1)
+ assert(literal.getJavaType === FieldDescriptor.JavaType.MESSAGE)
+ assert(literal.getMessageType.getFullName ===
"substrait.Expression.Literal")
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
