Flex types always converted correctly in cy_variant

Updated the logic in cy_variant.pyx to ensure that flexible types are
converted correctly.  Before, some types that should have been treated
as flexible types were converted to variant types instead.  This PR
ensures that such types are indeed converted correctly.  Tests included.

Author: Hoyt Koepke

oss_src/unity/python/sframe/cython/cy_flexible_type.pyx

@@ -1270,7 +1270,7 @@ cdef flexible_type _ft_translate(object v, int tr_code) except *:
 
     # These are optimized by the cython compiler into a big switch statement.
     if tr_code == FT_INT_TYPE:
-        ret.set_int(<long>v)
+        ret.set_int(<flex_int>v)
         return ret
     elif tr_code == FT_FLOAT_TYPE:
         ret.set_double(<double>v)

oss_src/unity/python/sframe/cython/cy_variant.pyx

@@ -22,10 +22,11 @@ from .cy_model cimport UnityModel
 from .cy_sframe cimport UnitySFrameProxy
 from .cy_sarray cimport UnitySArrayProxy
 
-from .cy_flexible_type cimport flexible_type, flex_list, flex_dict
+from .cy_flexible_type cimport flexible_type, flex_list, flex_dict, flex_int
 from .cy_flexible_type cimport flexible_type_from_pyobject
 from .cy_flexible_type cimport pyobject_from_flexible_type
 from .cy_flexible_type cimport check_list_to_vector_translation
+from .cy_flexible_type cimport flex_type_enum, UNDEFINED
 
 from .cy_dataframe cimport gl_dataframe
 from .cy_dataframe cimport is_pandas_dataframe
@@ -53,10 +54,10 @@ from libcpp.map cimport map
 
 # Fast track simple flexible types
 DEF VAR_TR_FT_INT                      = 0
-DEF VAR_TR_FT_LONG                     = 1
-DEF VAR_TR_FT_FLOAT                    = 2
-DEF VAR_TR_FT_STR                      = 3
-DEF VAR_TR_FT_UNICODE                  = 4
+DEF VAR_TR_FT_FLOAT                    = 1
+DEF VAR_TR_FT_STR                      = 2
+DEF VAR_TR_FT_UNICODE                  = 3
+DEF VAR_TR_FT_NONE                     = 4
 
 # Nested types
 DEF VAR_TR_DICT                        = 5
@@ -139,16 +140,18 @@ cdef int _get_tr_code_by_type_string(object v) except -1:
     if not internal_classes_set:
         import_internal_classes()
 
-    if type(v) is int:
+    if type(v) is int or type(v) is long:
         ret =  VAR_TR_FT_INT
-    elif type(v) is long:
-        ret =  VAR_TR_FT_LONG
     elif type(v) is float:
         ret =  VAR_TR_FT_FLOAT
     elif type(v) is str:
         ret =  VAR_TR_FT_STR
     elif type(v) is unicode_type:
         ret =  VAR_TR_FT_UNICODE
+    elif type(v) is bool:
+        ret =  VAR_TR_FT_INT
+    elif v is None:
+        ret =  VAR_TR_FT_NONE
     elif type(v) is list:
         ret =  VAR_TR_LIST
     elif type(v) is tuple:
@@ -306,10 +309,7 @@ cdef bint _var_set_listlike_internal(variant_vector_type& ret_as_vv,
         element_stored_in_flex_list = False
 
         if tr_code == VAR_TR_FT_INT:
-            ret_as_fl[i].set_int(<int>x)
-            element_stored_in_flex_list = True
-        elif tr_code == VAR_TR_FT_LONG:
-            ret_as_fl[i].set_int(<long>x)
+            ret_as_fl[i].set_int(<flex_int>x)
             element_stored_in_flex_list = True
         elif tr_code == VAR_TR_FT_FLOAT:
             ret_as_fl[i].set_double(<double>x)
@@ -320,6 +320,9 @@ cdef bint _var_set_listlike_internal(variant_vector_type& ret_as_vv,
         elif tr_code == VAR_TR_FT_UNICODE:
             ret_as_fl[i].set_string((<unicode>x))
             element_stored_in_flex_list = True
+        elif tr_code == VAR_TR_FT_NONE:
+            ret_as_fl[i] = flexible_type(UNDEFINED)
+            element_stored_in_flex_list = True
         elif tr_code == VAR_TR_LIST or tr_code == VAR_TR_TUPLE:
             ret_as_fl[i].set_list(flex_list())
 
@@ -406,8 +409,10 @@ cdef inline bint _var_set_dict_internal(variant_map_type& ret_as_vm, flex_dict&
 
     cdef long pos
     cdef int tr_code
-    cdef bint writing_to_flex_dict
-    
+    cdef bint writing_to_flex_dict, sub_is_flex_type
+    cdef variant_vector_type sub_vv = variant_vector_type()
+    cdef variant_map_type sub_vm = variant_map_type()
+
     if output_can_be_varmap:
         # First see if it can be translated as flexible type, which
         # would be great.
@@ -426,17 +431,65 @@ cdef inline bint _var_set_dict_internal(variant_map_type& ret_as_vm, flex_dict&
                     tr_code = get_var_tr_code(v)
 
                     if tr_code == VAR_TR_FT_INT:
-                        ret_as_fd[pos].second.set_int(<int>v)
-                    elif tr_code == VAR_TR_FT_LONG:
-                        ret_as_fd[pos].second.set_int(<long>v)
+                        ret_as_fd[pos].second.set_int(<flex_int>v)
                     elif tr_code == VAR_TR_FT_FLOAT:
                         ret_as_fd[pos].second.set_double(<double>v)
                     elif tr_code == VAR_TR_FT_STR:
                         ret_as_fd[pos].second.set_string(<str>v)
                     elif tr_code == VAR_TR_FT_UNICODE:
                         ret_as_fd[pos].second.set_string((<unicode>v))
+                    elif tr_code == VAR_TR_FT_NONE:
+                        ret_as_fd[pos].second = flexible_type(UNDEFINED)
                     elif tr_code == VAR_TR_ATTEMPT_OTHER_FLEXIBLE_TYPE:
                         ret_as_fd[pos].second = _translate_to_flexible_type(v)
+                    elif tr_code == VAR_TR_LIST or tr_code == VAR_TR_TUPLE:
+                        ret_as_fd[pos].second.set_list(flex_list())
+
+                        if tr_code == VAR_TR_LIST:
+                            sub_is_flex_type = _var_set_listlike_internal(
+                                sub_vv, ret_as_fd[pos].second.get_list_m(), <list>v, False)
+                        else:
+                            sub_is_flex_type = _var_set_listlike_internal(
+                                sub_vv, ret_as_fd[pos].second.get_list_m(), <tuple>v, False)
+
+                        if sub_is_flex_type:
+                            check_list_to_vector_translation(ret_as_fd[pos].second)
+
+                            if not writing_to_flex_dict:
+                                variant_set_flexible_type(ret_as_vm[ret_as_fd[pos].first.get_string()], 
+                                                          ret_as_fd[pos].second)
+                        else:                                                            
+                            if writing_to_flex_dict:
+                                ret_as_vm.clear()
+                                for i in range(pos):
+                                    variant_set_flexible_type(ret_as_vm[ret_as_fd[i].first.get_string()],
+                                                              ret_as_fd[i].second)
+
+                                writing_to_flex_dict = False
+
+                                
+                            variant_set_variant_vector(ret_as_vm[ret_as_fd[pos].first.get_string()], sub_vv)
+
+                    elif tr_code == VAR_TR_DICT:
+                        ret_as_fd[pos].second.set_dict(flex_dict())
+
+                        sub_is_flex_type = _var_set_dict_internal(sub_vm, ret_as_fd[pos].second.get_dict_m(), <dict>v, False)
+
+                        if sub_is_flex_type:
+                            if not writing_to_flex_dict:
+                                variant_set_flexible_type(ret_as_vm[ret_as_fd[pos].first.get_string()], 
+                                                          ret_as_fd[pos].second)
+                        else:
+                            if writing_to_flex_dict:
+                                ret_as_vm.clear()
+                                for i in range(pos):
+                                    variant_set_flexible_type(ret_as_vm[ret_as_fd[i].first.get_string()],
+                                                              ret_as_fd[i].second)
+
+                                writing_to_flex_dict = False
+
+                            variant_set_variant_map(ret_as_vm[ret_as_fd[pos].first.get_string()], sub_vm)
+                        
                     else:                    
                         # That didn't work -- translate the rest as
                         # variant type if it can be a varmap.
@@ -555,20 +608,19 @@ cdef _convert_to_variant_type(variant_type& ret, object v, int tr_code):
 
     # Fast-tracked flexible type versions
     if tr_code == VAR_TR_FT_INT:
-        ft.set_int(<int>v)
-        variant_set_flexible_type(ret, ft)
-    elif tr_code == VAR_TR_FT_LONG:
-        ft.set_int(<long>v)
+        ft.set_int(<flex_int>v)
         variant_set_flexible_type(ret, ft)
     elif tr_code == VAR_TR_FT_FLOAT:
-        ft.set_double(<float>v)
+        ft.set_double(<double>v)
         variant_set_flexible_type(ret, ft)
     elif tr_code == VAR_TR_FT_STR:
         ft.set_string(<str>v)
         variant_set_flexible_type(ret, ft)
     elif tr_code == VAR_TR_FT_UNICODE:
         ft.set_string(<unicode>v)
         variant_set_flexible_type(ret, ft)
+    elif tr_code == VAR_TR_FT_NONE:
+        variant_set_flexible_type(ret, flexible_type(UNDEFINED))
 
     # Nested container types
     elif tr_code == VAR_TR_DICT:
@@ -611,17 +663,16 @@ cdef _convert_to_variant_type(variant_type& ret, object v, int tr_code):
         variant_set_closure(ret, make_function_closure_info(v))
 
     # Flexible type -- this is actually the last resort since we
-    # assume the aboves are exact matches and do not include the
-    # flexible type stuff.
+    # assume the above types are exact matches and do not include the
+    # flexible type stuff, save for the fast paths for common types.
     elif tr_code == VAR_TR_ATTEMPT_OTHER_FLEXIBLE_TYPE:
         try:
             variant_set_flexible_type(ret, flexible_type_from_pyobject(v))
         except TypeError:
             raise_translation_error(v)
 
     else:
-        assert False
-
+        assert False    
 
 ################################################################################
 # The main translation function
@@ -693,3 +744,14 @@ cdef to_value(PyCommClient cli, variant_type& v):
         return to_vector(cli, variant_get_variant_vector(v))
     else:
         raise TypeError("Unsupported variant type.")
+
+################################################################################
+# Routines to assist with debugging. 
+    
+def _debug_is_flexible_type_encoded(object obj):
+    """
+    Checks to make sure that if an object can be encoded as a flexible
+    type, then it is.
+    """
+    cdef variant_type vt = from_value(obj)
+    return (vt.which() == VAR_TYPE_FLEXIBLE_TYPE)            

oss_src/unity/python/sframe/test/test_extensions.py

@@ -10,6 +10,8 @@
 from ..data_structures.sframe import SFrame
 import random
 
+from ..cython.cy_variant import _debug_is_flexible_type_encoded
+
 
 class VariantCheckTest(unittest.TestCase):
 
@@ -67,6 +69,10 @@ def test_stress(self):
 
         random.seed(0)
 
+
+        class A: pass
+        A.flextype_encodable = True
+
         def _make(depth):
 
             if depth == 0:
@@ -79,8 +85,10 @@ def _make(depth):
             elif s == 1:
                 return random.randint(0,100000)
             elif s == 2:
+                A.flextype_encodable = False
                 return SArray([random.randint(0,100000) for i in range(2)])
             elif s == 3:
+                A.flextype_encodable = False
                 return SFrame({'a' : [random.randint(0,100000) for i in range(2)],
                                'b' : [str(random.randint(0,100000)) for i in range(2)]})
 
@@ -93,5 +101,18 @@ def _make(depth):
                 return {str(random.randint(0, 100)) : _make(depth - 1)
                         for i in range(length)}
 
-        for i in range(10):
-            self.variant_turnaround(_make(5 + i))
+        for depth in [2,3,4,5,10]:
+            for i in range(10):
+                
+                A.flextype_encodable = True
+
+                obj = _make(depth)
+
+                # Test that it's losslessly encoded and decoded
+                self.variant_turnaround(obj)
+
+                # Test that if it can be 
+                if _debug_is_flexible_type_encoded(obj):
+                    self.assertTrue(A.flextype_encodable)
+                else:
+                    self.assertFalse(A.flextype_encodable)