fix: detect when maps are missing keys and/or values (#65)

Now that the underlying type library doesn't segfault when given a map missing a key or value, properly detect such cases and return errors.
substrait-io · Jun 1, 2023 · d995094 · d995094
1 parent 1605e89
commit d995094
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 6 deletions.
diff --git a/src/substrait/textplan/parser/SubstraitPlanRelationVisitor.cpp b/src/substrait/textplan/parser/SubstraitPlanRelationVisitor.cpp
@@ -1276,6 +1276,11 @@ ::substrait::proto::Type SubstraitPlanRelationVisitor::typeToProto(
     }
     case TypeKind::kMap: {
       auto map = reinterpret_cast<const ParameterizedMap*>(&decodedType);
+      if (map->keyType() == nullptr || map->valueType() == nullptr) {
+        errorListener_->addError(
+            token, "Maps require both a key and a value type.");
+        break;
+      }
       *type.mutable_map()->mutable_key() = typeToProto(token, *map->keyType());
       *type.mutable_map()->mutable_value() =
           typeToProto(token, *map->valueType());

diff --git a/src/substrait/textplan/parser/grammar/SubstraitPlanParser.g4 b/src/substrait/textplan/parser/grammar/SubstraitPlanParser.g4
@@ -90,7 +90,7 @@ literal_basic_type
 literal_complex_type
    : literal_basic_type
    | LIST QUESTIONMARK? LEFTANGLEBRACKET literal_complex_type? RIGHTANGLEBRACKET
-   | MAP QUESTIONMARK? LEFTANGLEBRACKET (literal_basic_type COMMA literal_complex_type)? RIGHTANGLEBRACKET
+   | MAP QUESTIONMARK? LEFTANGLEBRACKET literal_basic_type? COMMA? literal_complex_type? RIGHTANGLEBRACKET
    | STRUCT QUESTIONMARK? LEFTANGLEBRACKET literal_complex_type? (COMMA literal_complex_type)* RIGHTANGLEBRACKET
    ;
 

diff --git a/src/substrait/textplan/parser/tests/TextPlanParserTest.cpp b/src/substrait/textplan/parser/tests/TextPlanParserTest.cpp
@@ -652,7 +652,10 @@ std::vector<TestCase> getTestCases() {
             expression {}_list<string>?;
             expression {}_struct<a>;
             expression {}_struct<>;
-            //expression {}_map<>;  // TODO - Catch this behavior.
+            expression {}_map<>;
+            expression {}_map<string>;
+            expression {}_map<,string>;
+            expression {}_map<,>;
             expression {}_list<>;
             expression "unknown\escape"_string;
             expression {123_i8}_map<i8, bool>;
@@ -682,10 +685,17 @@ std::vector<TestCase> getTestCases() {
               "11:23 → Could not parse literal as decimal.",
               "13:26 → Unable to recognize requested type.",
               "14:26 → Unable to recognize requested type.",
-              "16:26 → Unable to recognize requested type.",
-              "17:31 → Unknown slash escape sequence.",
-              "18:23 → Map literals require pairs of values separated by colons.",
-              "19:23 → Map literals require pairs of values separated by colons.",
+              "15:26 → Maps require both a key and a value type.",
+              "15:23 → Unsupported type 0.",
+              "16:26 → Maps require both a key and a value type.",
+              "16:23 → Unsupported type 0.",
+              "17:26 → Unable to recognize requested type.",
+              "18:26 → Unable to recognize requested type.",
+              "18:26 → Unable to recognize requested type.",
+              "19:26 → Unable to recognize requested type.",
+              "20:31 → Unknown slash escape sequence.",
+              "21:23 → Map literals require pairs of values separated by colons.",
+              "22:23 → Map literals require pairs of values separated by colons.",
           }),
       },
       {