Merge pull request #18 from github/optional_fields

Don't generate an index for optional fields that occur at most once

Author: nickrolfe

extractor/src/extractor.rs

@@ -230,13 +230,23 @@ impl Visitor<'_> {
                         )
                     }
                 }
-                Storage::Table => {
+                Storage::Table(has_index) => {
                     for (index, child_id) in child_ids.iter().enumerate() {
+                        if !*has_index && index > 0 {
+                            error!(
+                                "{}:{}: too many values for field: {}::{}",
+                                &self.path,
+                                node.start_position().row,
+                                node.kind(),
+                                &field.get_name()
+                            );
+                            break;
+                        }
                         self.trap_output.push(TrapEntry::ChildOf(
                             node_type_name(&field.parent.kind, field.parent.named),
                             parent_id,
                             field.get_name(),
-                            Index(index),
+                            if *has_index { Some(Index(index)) } else { None },
                             *child_id,
                         ));
                     }
@@ -364,7 +374,7 @@ enum TrapEntry {
     // @node_def(self, arg?, location)@
     Definition(String, Label, Vec<Arg>, Label),
     // @node_child(self, index, parent)@
-    ChildOf(String, Label, String, Index, Label),
+    ChildOf(String, Label, String, Option<Index>, Label),
     // @location(loc, path, r1, c1, r2, c2)
     Located(Vec<Arg>),
     Comment(String),
@@ -387,14 +397,23 @@ impl fmt::Display for TrapEntry {
                     loc
                 )
             }
-            TrapEntry::ChildOf(pname, id, fname, idx, p) => write!(
-                f,
-                "{}({}, {}, {})",
-                escape_name(&format!("{}_{}", &pname, &fname)),
-                id,
-                idx,
-                p
-            ),
+            TrapEntry::ChildOf(pname, id, fname, idx, p) => match idx {
+                Some(idx) => write!(
+                    f,
+                    "{}({}, {}, {})",
+                    escape_name(&format!("{}_{}", &pname, &fname)),
+                    id,
+                    idx,
+                    p
+                ),
+                None => write!(
+                    f,
+                    "{}({}, {})",
+                    escape_name(&format!("{}_{}", &pname, &fname)),
+                    id,
+                    p
+                ),
+            },
             TrapEntry::Located(args) => write!(
                 f,
                 "location({}, {}, {}, {}, {}, {})",

generator/src/main.rs

@@ -49,45 +49,49 @@ fn add_field(
 ) {
     let field_name = field.get_name();
     let parent_name = node_types::node_type_name(&field.parent.kind, field.parent.named);
-    match field.storage {
-        node_types::Storage::Table { .. } => {
+    match &field.storage {
+        node_types::Storage::Table(has_index) => {
             // This field can appear zero or multiple times, so put
             // it in an auxiliary table.
             let field_type = make_field_type(&parent_name, &field_name, &field.types, entries);
+            let parent_column = dbscheme::Column {
+                unique: !*has_index,
+                db_type: dbscheme::DbColumnType::Int,
+                name: node_types::escape_name(&parent_name),
+                ql_type: ql::Type::AtType(node_types::escape_name(&parent_name)),
+                ql_type_is_ref: true,
+            };
+            let index_column = dbscheme::Column {
+                unique: false,
+                db_type: dbscheme::DbColumnType::Int,
+                name: "index".to_string(),
+                ql_type: ql::Type::Int,
+                ql_type_is_ref: true,
+            };
+            let field_column = dbscheme::Column {
+                unique: true,
+                db_type: dbscheme::DbColumnType::Int,
+                name: node_types::escape_name(&field_type),
+                ql_type: ql::Type::AtType(field_type),
+                ql_type_is_ref: true,
+            };
             let field_table = dbscheme::Table {
                 name: format!("{}_{}", parent_name, field_name),
-                columns: vec![
-                    // First column is a reference to the parent.
-                    dbscheme::Column {
-                        unique: false,
-                        db_type: dbscheme::DbColumnType::Int,
-                        name: node_types::escape_name(&parent_name),
-                        ql_type: ql::Type::AtType(node_types::escape_name(&parent_name)),
-                        ql_type_is_ref: true,
-                    },
-                    // Then an index column.
-                    dbscheme::Column {
-                        unique: false,
-                        db_type: dbscheme::DbColumnType::Int,
-                        name: "index".to_string(),
-                        ql_type: ql::Type::Int,
-                        ql_type_is_ref: true,
-                    },
-                    // And then the field
-                    dbscheme::Column {
-                        unique: true,
-                        db_type: dbscheme::DbColumnType::Int,
-                        name: node_types::escape_name(&field_type),
-                        ql_type: ql::Type::AtType(field_type),
-                        ql_type_is_ref: true,
-                    },
-                ],
+                columns: if *has_index {
+                    vec![parent_column, index_column, field_column]
+                } else {
+                    vec![parent_column, field_column]
+                },
                 // In addition to the field being unique, the combination of
                 // parent+index is unique, so add a keyset for them.
-                keysets: Some(vec![
-                    node_types::escape_name(&parent_name),
-                    "index".to_string(),
-                ]),
+                keysets: if *has_index {
+                    Some(vec![
+                        node_types::escape_name(&parent_name),
+                        "index".to_string(),
+                    ])
+                } else {
+                    None
+                },
             };
             entries.push(dbscheme::Entry::Table(field_table));
         }

generator/src/ql_gen.rs

@@ -339,15 +339,21 @@ fn create_get_field_expr_for_column_storage(
 /// all indices.
 fn create_get_field_expr_for_table_storage(
     table_name: &str,
-    index_var_name: &str,
+    index_var_name: Option<&str>,
 ) -> ql::Expression {
     ql::Expression::Pred(
         table_name.to_owned(),
-        vec![
-            ql::Expression::Var("this".to_owned()),
-            ql::Expression::Var(index_var_name.to_owned()),
-            ql::Expression::Var("result".to_owned()),
-        ],
+        match index_var_name {
+            Some(index_var_name) => vec![
+                ql::Expression::Var("this".to_owned()),
+                ql::Expression::Var(index_var_name.to_owned()),
+                ql::Expression::Var("result".to_owned()),
+            ],
+            None => vec![
+                ql::Expression::Var("this".to_owned()),
+                ql::Expression::Var("result".to_owned()),
+            ],
+        },
     )
 }
 
@@ -400,7 +406,7 @@ fn create_field_getters(
             *main_table_column_index += 1;
             result
         }
-        node_types::Storage::Table => {
+        node_types::Storage::Table(has_index) => {
             let field_table_name = format!("{}_{}", parent_name, &field.get_name());
             (
                 ql::Predicate {
@@ -410,13 +416,23 @@ fn create_field_getters(
                     ),
                     overridden: false,
                     return_type: Some(ql::Type::Normal(dbscheme_name_to_class_name(field_type))),
-                    formal_parameters: vec![ql::FormalParameter {
-                        name: "i".to_owned(),
-                        param_type: ql::Type::Int,
-                    }],
-                    body: create_get_field_expr_for_table_storage(&field_table_name, "i"),
+                    formal_parameters: if *has_index {
+                        vec![ql::FormalParameter {
+                            name: "i".to_owned(),
+                            param_type: ql::Type::Int,
+                        }]
+                    } else {
+                        vec![]
+                    },
+                    body: create_get_field_expr_for_table_storage(
+                        &field_table_name,
+                        if *has_index { Some("i") } else { None },
+                    ),
                 },
-                create_get_field_expr_for_table_storage(&field_table_name, "_"),
+                create_get_field_expr_for_table_storage(
+                    &field_table_name,
+                    if *has_index { Some("_") } else { None },
+                ),
             )
         }
     }

node-types/src/lib.rs

@@ -46,8 +46,9 @@ impl Field {
 pub enum Storage {
     /// the field is stored as a column in the parent table
     Column,
-    // the field is store in a link table
-    Table,
+    /// the field is stored in a link table, and may or may not have an
+    /// associated index column
+    Table(bool),
 }
 
 pub fn read_node_types(node_types_path: &Path) -> std::io::Result<Vec<Entry>> {
@@ -122,16 +123,19 @@ fn add_field(
     field_info: &FieldInfo,
     fields: &mut Vec<Field>,
 ) {
-    let storage;
-    if !field_info.multiple && field_info.required {
+    let storage = if !field_info.multiple && field_info.required {
         // This field must appear exactly once, so we add it as
         // a column to the main table for the node type.
-        storage = Storage::Column;
+        Storage::Column
+    } else if !field_info.multiple {
+        // This field is optional but can occur at most once. Put it in an
+        // auxiliary table without an index.
+        Storage::Table(false)
     } else {
-        // This field can appear zero or multiple times, so put
-        // it in an auxiliary table.
-        storage = Storage::Table;
-    }
+        // This field can occur multiple times. Put it in an auxiliary table
+        // with an associated index.
+        Storage::Table(true)
+    };
     fields.push(Field {
         parent: TypeName {
             kind: parent_type_name.kind.to_string(),