Addressed review comments.

src/substrait/textplan/StructuredSymbolData.h

@@ -41,15 +41,18 @@ struct RelationData {
  // Each field reference here was generated within the current relation.
  std::vector<const SymbolInfo*> generatedFieldReferences;
 
- // Local aliases for field references in this relation.
+ // Local aliases for field references in this relation. Used to replace the
+ // normal form symbols would take for this relation's use only. (Later
+ // references to the symbol would use the alias.)
  std::map<size_t, std::string> generatedFieldReferenceAlternativeExpression;
 
  // If populated, supersedes the combination of fieldReferences and
  // generatedFieldReferences for the field symbols exposed by this relation.
  std::vector<const SymbolInfo*> outputFieldReferences;
 
- // Contains the field reference names seen so far along with the id of the
- // first occurrence.
+ // Contains the field reference names seen so far while processing this
+ // relation along with the id of the first occurrence. Used to detect when
+ // fully qualified references are necessary.
  std::map<std::string, size_t> seenFieldReferenceNames;
 };
 

src/substrait/textplan/converter/InitialPlanProtoVisitor.cpp

@@ -19,7 +19,7 @@ namespace io::substrait::textplan {
 
 namespace {
 
-const std::string kIntermediateNodeName = "intermediate";
+const std::string kIntermediateNodeName{"intermediate"};
 const std::string kRootNames{"root.names"};
 
 std::string shortName(std::string str) {
@@ -90,7 +90,6 @@ ::google::protobuf::RepeatedField<int32_t> getOutputMapping(
  return relation.filter().common().emit().output_mapping();
  case ::substrait::proto::Rel::kFetch:
  return relation.fetch().common().emit().output_mapping();
- ;
  case ::substrait::proto::Rel::kAggregate:
  return relation.aggregate().common().emit().output_mapping();
  case ::substrait::proto::Rel::kSort:
@@ -100,7 +99,7 @@ ::google::protobuf::RepeatedField<int32_t> getOutputMapping(
  case ::substrait::proto::Rel::kProject:
  return relation.project().common().emit().output_mapping();
  case ::substrait::proto::Rel::kSet:
- return relation.sort().common().emit().output_mapping();
+ return relation.set().common().emit().output_mapping();
  case ::substrait::proto::Rel::kExtensionSingle:
  return relation.extension_single().common().emit().output_mapping();
  case ::substrait::proto::Rel::kExtensionMulti:
@@ -330,6 +329,7 @@ std::any InitialPlanProtoVisitor::visitNamedStruct(
  SymbolType::kField,
  SourceType::kUnknown,
  type);
+ nameNum++;
  }
  return BasePlanProtoVisitor::visitNamedStruct(named);
 }

src/substrait/textplan/converter/PipelineVisitor.cpp

@@ -118,6 +118,7 @@ std::any PipelineVisitor::visitPlanRelation(
  const ::substrait::proto::PlanRel& relation) {
  auto symbols =
  symbolTable_->lookupSymbolsByLocation(PROTO_LOCATION(relation));
+ // A symbol is guaranteed as we previously visited the parse tree.
  auto relationData = ANY_CAST(std::shared_ptr<RelationData>, symbols[0]->blob);
  switch (relation.rel_type_case()) {
  case ::substrait::proto::PlanRel::kRel: {

src/substrait/textplan/parser/SubstraitPlanRelationVisitor.cpp

@@ -278,10 +278,12 @@ void addInputFieldsToSchema(
  auto continuingRelationData = ANY_CAST(
  std::shared_ptr<RelationData>, relationData->continuingPipeline->blob);
  if (!continuingRelationData->outputFieldReferences.empty()) {
+ // There is an emit sequence so use that.
  for (auto field : continuingRelationData->outputFieldReferences) {
  relationData->fieldReferences.push_back(field);
  }
  } else {
+ // There was no emit so just access all the field references.
  for (auto field : continuingRelationData->fieldReferences) {
  relationData->fieldReferences.push_back(field);
  }
@@ -594,7 +596,7 @@ std::any SubstraitPlanRelationVisitor::visitRelationUsesSchema(
  auto typeProto = ANY_CAST(::substrait::proto::Type, sym.blob);
  if (typeProto.kind_case() != ::substrait::proto::Type::KIND_NOT_SET) {
  *schema->mutable_struct_()->add_types() = typeProto;
- // If the schema contains one more types, the structure is required.
+ // If the schema contains any types, the struct is required.
  schema->mutable_struct_()->set_nullability(
  ::substrait::proto::Type_Nullability_NULLABILITY_REQUIRED);
  }
@@ -1161,8 +1163,6 @@ std::any SubstraitPlanRelationVisitor::visitExpressionColumn(
  // TODO -- Update the following when non-direct references are implemented.
  expr.mutable_selection()->mutable_root_reference();
  }
-
- // visitChildren(ctx);
  return expr;
 }
 
@@ -1990,6 +1990,7 @@ void SubstraitPlanRelationVisitor::applyOutputMappingToSchema(
  common->mutable_direct();
  } else {
  if (!relationData->outputFieldReferences.empty()) {
+ // TODO -- Add support for aggregate relations.
  errorListener_->addError(
  token, "Aggregate relations do not yet support emit sections.");
  return;

src/substrait/textplan/parser/SubstraitPlanVisitor.cpp

@@ -511,7 +511,6 @@ std::any SubstraitPlanVisitor::visitFile_detail(
  } else {
  return visitChildren(ctx);
  }
- // symbol->blob.swap(item);
  return defaultResult();
 }