Duplicate columns are ignored when adding default View columns and annotation columns to EntityView(SYNPY-489) (#503)

zimingd · web-flow · commit d8761956e023 · 2018-01-25T14:05:25.000-08:00
* reafactored EntityView repeated column logic

* flitering of repeated column names added for both annotations and default view columns
diff --git a/synapseclient/table.py b/synapseclient/table.py
@@ -551,6 +551,7 @@ def _before_synapse_store(self, syn):
             self.properties.columnIds.extend(column.id for column in syn.createColumns(self.columns_to_store))
             self.columns_to_store = []
 
+
 class Schema(SchemaBase):
     """
     A Schema is an :py:class:`synapseclient.entity.Entity` that defines a set of columns in a table.
@@ -645,61 +646,62 @@ def add_scope(self, entities):
             self.scopeIds.append(id_of(entities))
 
     def _before_synapse_store(self, syn):
-        if self.addAnnotationColumns:
-            self._add_annotations_as_columns(syn)
-            self.addAnnotationColumns = False
-
         #get the default EntityView columns from Synapse and add them to the columns list
+        additional_columns = []
         if self.addDefaultViewColumns:
-            self.addColumns(syn._get_default_entity_view_columns(self.type))
-            self.addDefaultViewColumns = False
+            additional_columns.extend(syn._get_default_entity_view_columns(self.type))
+
+        #get default annotations
+        if self.addAnnotationColumns:
+            anno_columns = [x for x in syn._get_annotation_entity_view_columns(self.scopeIds, self.type) if
+             x['name'] not in self.ignoredAnnotationColumnNames]
+            additional_columns.extend(anno_columns)
+
+        self.addColumns(self._filter_duplicate_columns(syn, additional_columns))
+
+        #set these boolean flags to false so they are not repeated.
+        self.addDefaultViewColumns = False
+        self.addAnnotationColumns = False
 
         super(EntityViewSchema, self)._before_synapse_store(syn)
 
 
-    def _add_annotations_as_columns(self, syn):
-        column_type_to_annotation_names = {
-            'STRING': set(),
-            'INTEGER': set(),
-            'DOUBLE': set(),
-            'DATE': set()
-        }
-        all_existing_column_names = set() # set of all existing columns names regardless of type
+    def _filter_duplicate_columns(self, syn, columns_to_add):
+        """
+        If a column to be added has the same name and same type as an existing column, it will be considered a duplicate and not added.
+        :param syn: a :py:class:`synapseclient.client.Synapse` object that is logged in
+        :param columns_to_add: iterable collection of type :py:class:`synapseclient.table.Column` objects
+        :return: a filtered list of columns to add
+        """
+
+        # no point in making HTTP calls to retrieve existing Columns if we not adding any new columns
+        if not columns_to_add:
+            return columns_to_add
+
+        # set up Column name/type tracking
+        column_type_to_annotation_names = {} # map of str -> set(str), where str is the column type as a string and set is a set of column name strings
 
         # add to existing columns the columns that user has added but not yet created in synapse
-        column_generator = itertools.chain(syn.getColumns(self.columnIds), self.columns_to_store) if self.columns_to_store else syn.getColumns(self.columnIds)
+        column_generator = itertools.chain(syn.getColumns(self.columnIds),
+                                           self.columns_to_store) if self.columns_to_store else syn.getColumns(self.columnIds)
 
         for column in column_generator:
             column_name = column['name']
             column_type = column['columnType']
 
-            all_existing_column_names.add(column_name)
-            #add to type specific set
-            if column_type in column_type_to_annotation_names:
-                column_type_to_annotation_names[column_type].add(column_name)
-
-
-        #get annotations from each of the scopes and create columns
-        columns_to_add = [] #temporarily store all columns so that none are added if any errors occur
-        anno_columns = syn._get_annotation_entity_view_columns(self.scopeIds, self.type)
-        for column in anno_columns:
-            anno_col_name = column['name']
-            anno_col_type = column['columnType']
-            typed_col_name_set = column_type_to_annotation_names[anno_col_type]
-            if (anno_col_name not in self.ignoredAnnotationColumnNames
-                 and anno_col_name not in typed_col_name_set):
-
-                if anno_col_name in all_existing_column_names:
-                    raise ValueError("The annotation column name [%s] has multiple types in your scopes or in your defined columns.\n"
-                                     "Please do one of the following:\n"
-                                     "  Turn off the automatic conversion of annotations to column names: entityView.addAnnotationColumns = False\n"
-                                     "  Modify your annotations/columns named [%s] to all be of the same type.\n"
-                                     "  Add the annotation name to the set of ignored annotation names via entityView.ignoredAnnotations.add(%s).\n" % (anno_col_name, anno_col_name, anno_col_name))
-                all_existing_column_names.add(anno_col_name)
-                typed_col_name_set.add(anno_col_name)
-                columns_to_add.append(column)
-        self.addColumns(columns_to_add)
+            column_type_to_annotation_names.setdefault(column_type,set()).add(column_name)
+
+
+        valid_columns = []
+        for column in columns_to_add:
+            new_col_name = column['name']
+            new_col_type = column['columnType']
 
+            typed_col_name_set = column_type_to_annotation_names.setdefault(new_col_type, set())
+            if new_col_name not in typed_col_name_set:
+                typed_col_name_set.add(new_col_name)
+                valid_columns.append(column)
+        return valid_columns
 
 
 ## add Schema to the map of synapse entity types to their Python representations
diff --git a/tests/integration/test_tables.py b/tests/integration/test_tables.py
@@ -159,6 +159,7 @@ def test_entity_view_add_annotation_columns():
     prev_columns = list(entity_view.columnIds)
     # sometimes annotation columns are not immediately updated so we wait for it to update in a loop
     while len(entity_view.columnIds) != len(prev_columns) + 1:
+        print(len(prev_columns), entity_view.columnIds)
         entity_view.addAnnotationColumns = True
         entity_view = syn.store(entity_view)
 
diff --git a/tests/unit/unit_test_tables.py b/tests/unit/unit_test_tables.py
@@ -29,7 +29,7 @@
 from synapseclient.exceptions import SynapseError
 from synapseclient.entity import split_entity_namespaces
 from synapseclient.table import Column, Schema, CsvFileTable, TableQueryResult, cast_values, \
-     as_table_columns, Table, RowSet, SelectColumn, EntityViewSchema, RowSetTable, Row, PartialRow, PartialRowset
+     as_table_columns, Table, RowSet, SelectColumn, EntityViewSchema, RowSetTable, Row, PartialRow, PartialRowset, SchemaBase
 from mock import patch
 from collections import OrderedDict
 
@@ -610,40 +610,58 @@ def test_EntityViewSchema__ignore_column_names_set_info_preserved():
 
 
 
-def test_EntityViewSchema__ignore_column_names():
+def test_EntityViewSchema__ignore_annotation_column_names():
     syn = synapseclient.client.Synapse(debug=True, skip_checks=True)
 
     scopeIds = ['123']
-    entity_view = EntityViewSchema("someName", scopes = scopeIds ,parent="syn123", ignoredAnnotationColumnNames={'long1'})
+    entity_view = EntityViewSchema("someName", scopes = scopeIds ,parent="syn123", ignoredAnnotationColumnNames={'long1'}, addDefaultViewColumns=False, addAnnotationColumns=True)
 
     mocked_annotation_result1 = [Column(name='long1', columnType='INTEGER'), Column(name='long2', columnType ='INTEGER')]
 
     with patch.object(syn, '_get_annotation_entity_view_columns', return_value=mocked_annotation_result1) as mocked_get_annotations,\
-         patch.object(syn, 'getColumns') as mocked_get_columns:
+         patch.object(syn, 'getColumns') as mocked_get_columns,\
+         patch.object(SchemaBase, "_before_synapse_store"):
 
-        entity_view._add_annotations_as_columns(syn)
+        entity_view._before_synapse_store(syn)
 
         mocked_get_columns.assert_called_once_with([])
         mocked_get_annotations.assert_called_once_with(scopeIds, 'file')
 
         assert_equals([Column(name='long2', columnType='INTEGER')], entity_view.columns_to_store)
 
 
-def test_EntityViewSchema__repeated_columnName():
+def test_EntityViewSchema__repeated_columnName_different_type():
     syn = synapseclient.client.Synapse(debug=True, skip_checks=True)
 
     scopeIds = ['123']
     entity_view = EntityViewSchema("someName", scopes = scopeIds ,parent="syn123")
 
-    mocked_annotation_result1 = [Column(name='annoName', columnType='INTEGER'), Column(name='annoName', columnType='DOUBLE')]
+    columns = [Column(name='annoName', columnType='INTEGER'),
+                                 Column(name='annoName', columnType='DOUBLE')]
 
-    with patch.object(syn, '_get_annotation_entity_view_columns', return_value=mocked_annotation_result1) as mocked_get_annotations,\
-         patch.object(syn, 'getColumns') as mocked_get_columns:
+    with patch.object(syn, 'getColumns') as mocked_get_columns:
 
-        assert_raises(ValueError, entity_view._add_annotations_as_columns, syn)
+        filtered_results = entity_view._filter_duplicate_columns(syn, columns)
 
         mocked_get_columns.assert_called_once_with([])
-        mocked_get_annotations.assert_called_once_with(scopeIds, 'file')
+        assert_equals(2, len(filtered_results))
+        assert_equals(columns, filtered_results)
+
+
+def test_EntityViewSchema__repeated_columnName_same_type():
+    syn = synapseclient.client.Synapse(debug=True, skip_checks=True)
+
+    entity_view = EntityViewSchema("someName", parent="syn123")
+
+    columns = [Column(name='annoName', columnType='INTEGER'),
+               Column(name='annoName', columnType='INTEGER')]
+
+    with patch.object(syn, 'getColumns') as mocked_get_columns:
+        filtered_results = entity_view._filter_duplicate_columns(syn, columns)
+
+        mocked_get_columns.assert_called_once_with([])
+        assert_equals(1, len(filtered_results))
+        assert_equals(Column(name='annoName', columnType='INTEGER'), filtered_results[0])
 
 
 def test_rowset_asDataFrame__with_ROW_ETAG_column():
