diff --git a/cpp/velox/substrait/SubstraitToVeloxPlan.cc b/cpp/velox/substrait/SubstraitToVeloxPlan.cc index 2cff687dd2..5a540ce2cd 100644 --- a/cpp/velox/substrait/SubstraitToVeloxPlan.cc +++ b/cpp/velox/substrait/SubstraitToVeloxPlan.cc @@ -1681,26 +1681,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 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 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); @@ -1709,7 +1706,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(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 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& 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& 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& 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") + } +}