Booleans are no longer _automatically_ coerced with numbers. Adding t…

…ype unification checks to `iif`, `fill_nothing` and `coalesce`.
enso-org · Apr 18, 2023 · 8c87729 · 8c87729
1 parent ef3858f
commit 8c87729
Show file tree

Hide file tree

Showing 13 changed files with 176 additions and 132 deletions.
diff --git a/distribution/lib/Standard/Database/0.0.0-dev/src/Data/Column.enso b/distribution/lib/Standard/Database/0.0.0-dev/src/Data/Column.enso
@@ -548,8 +548,10 @@ type Column
     iif : Any -> Any -> Column
     iif self when_true when_false =
         type_helpers.check_argument_type self Value_Type.expect_boolean <|
-            new_name = "if " + self.naming_helpers.to_expression_text self + " then " + self.naming_helpers.to_expression_text when_true + " else " + self.naming_helpers.to_expression_text when_false
-            self.make_op "IIF" [when_true, when_false] new_name
+            common_type = type_helpers.find_common_type_for_arguments [when_true, when_false]
+            common_type.if_not_error <|
+                new_name = "if " + self.naming_helpers.to_expression_text self + " then " + self.naming_helpers.to_expression_text when_true + " else " + self.naming_helpers.to_expression_text when_false
+                self.make_op "IIF" [when_true, when_false] new_name
 
     ## Returns a column of first non-`Nothing` value on each row of `self` and
        `values` list.
@@ -564,12 +566,13 @@ type Column
 
              example_coalesce = Examples.decimal_column.coalesce Examples.integer_column
     coalesce : (Any | Vector Any) -> Column
-    coalesce self values = case values of
-        _ : Vector ->
-            new_name = self.naming_helpers.function_name "coalesce" [self]+values
-            self.make_op "COALESCE" values new_name
-        _ : Array -> self.coalesce (Vector.from_polyglot_array values)
-        _ -> self.coalesce [values]
+    coalesce self values =
+        vec = Vector.unify_vector_or_element values
+        args_with_self = [self]+vec
+        common_type = type_helpers.find_common_type_for_arguments args_with_self
+        common_type.if_not_error <|
+            new_name = self.naming_helpers.function_name "coalesce" args_with_self
+            self.make_op "COALESCE" vec new_name
 
     ## Returns a column of minimum on each row of `self` and `values`.
 
@@ -676,8 +679,10 @@ type Column
        provided default.
     fill_nothing : Column | Any -> Column
     fill_nothing self default =
-        new_name = self.naming_helpers.function_name "fill_nothing" [self, default]
-        self.make_binary_op "FILL_NULL" default new_name
+        common_type = type_helpers.find_common_type_for_arguments [self, default]
+        common_type.if_not_error <|
+            new_name = self.naming_helpers.function_name "fill_nothing" [self, default]
+            self.make_binary_op "FILL_NULL" default new_name
 
     ## ALIAS Fill Empty
 

diff --git a/distribution/lib/Standard/Table/0.0.0-dev/src/Data/Column.enso b/distribution/lib/Standard/Table/0.0.0-dev/src/Data/Column.enso
@@ -645,19 +645,25 @@ type Column
     iif : Any -> Any -> Column
     iif self when_true when_false =
         type_helpers.check_argument_type self Value_Type.expect_boolean <|
-            new_name = "if " + Naming_Helpers.to_expression_text self + " then " + Naming_Helpers.to_expression_text when_true + " else " + Naming_Helpers.to_expression_text when_false
-            s = self.java_column.getStorage
+            common_type = type_helpers.find_common_type_for_arguments [when_true, when_false]
+            storage_type = Storage.from_value_type common_type Problem_Behavior.Report_Error
+            storage_type.catch Inexact_Type_Coercion error->
+                Panic.throw (Illegal_State.Error "Common type "+common_type.to_display_text+" is not supported by the Storage. This should never happen and is a bug in the Table library.")
 
-            true_val = case when_true of
-                _ : Column -> when_true.java_column.getStorage
-                _ -> when_true
+            storage_type.if_not_error <|
+                new_name = "if " + Naming_Helpers.to_expression_text self + " then " + Naming_Helpers.to_expression_text when_true + " else " + Naming_Helpers.to_expression_text when_false
+                s = self.java_column.getStorage
 
-            false_val = case when_false of
-                _ : Column -> when_false.java_column.getStorage
-                _ -> when_false
+                true_val = case when_true of
+                    _ : Column -> when_true.java_column.getStorage
+                    _ -> when_true
 
-            rs = s.iif true_val false_val
-            Column.Value (Java_Column.new new_name rs)
+                false_val = case when_false of
+                    _ : Column -> when_false.java_column.getStorage
+                    _ -> when_false
+
+                rs = s.iif true_val false_val storage_type
+                Column.Value (Java_Column.new new_name rs)
 
     ## Returns a column of first non-`Nothing` value on each row of `self` and
        `values` list.
@@ -814,16 +820,18 @@ type Column
              example_fill_missing = Examples.decimal_column.fill_nothing 20.5
     fill_nothing : Column | Any -> Column
     fill_nothing self default =
-        new_name = Naming_Helpers.function_name "fill_nothing" [self, default]
-        storage = self.java_column.getStorage
-        new_st = case default of
-            Column.Value java_col ->
-                other_storage = java_col.getStorage
-                storage.fillMissingFrom other_storage
-            _ ->
-                storage.fillMissing default
-        col = Java_Column.new new_name new_st
-        Column.Value col
+        common_type = type_helpers.find_common_type_for_arguments [self, default]
+        common_type.if_not_error <|
+            new_name = Naming_Helpers.function_name "fill_nothing" [self, default]
+            storage = self.java_column.getStorage
+            new_st = case default of
+                Column.Value java_col ->
+                    other_storage = java_col.getStorage
+                    storage.fillMissingFrom other_storage
+                _ ->
+                    storage.fillMissing default
+            col = Java_Column.new new_name new_st
+            Column.Value col
 
     ## ALIAS Fill Empty
 
@@ -1450,26 +1458,32 @@ type Column
    - name: The name of the vectorized operation.
    - fallback_fn: A function used if the vectorized operation isn't available.
    - operands: The vector of operands to apply to the function after `column`.
+   - new_name: The name of the column created as the result of this operation.
    - skip_nulls: Specifies if nulls should be skipped. If set to `True`, a null
      value results in null without passing it to the function. If set to
      `False`, the null values are passed as any other value and can have custom
      handling logic.
-   - new_name: The name of the column created as the result of this operation.
+   - check_common_type: If True, the method will check if a common type can be
+     found for all arguments.
 run_vectorized_many_op : Column -> Text -> (Any -> Any -> Any) -> Vector -> Text|Nothing -> Boolean -> Column
-run_vectorized_many_op column name fallback_fn operands new_name=Nothing skip_nulls=False =
+run_vectorized_many_op column name fallback_fn operands new_name=Nothing skip_nulls=False check_common_type=True =
     effective_operands = Vector.unify_vector_or_element operands
     effective_new_name = new_name.if_nothing <|
         Naming_Helpers.function_name name [column]+effective_operands
-    problem_builder = MapOperationProblemBuilder.new effective_new_name
-    folded = effective_operands.fold column.java_column.getStorage current-> operand->
-        case operand of
-            _ : Column ->
-                current.zip name fallback_fn operand.java_column.getStorage skip_nulls problem_builder
-            _ ->
-                current.bimap name fallback_fn operand skip_nulls problem_builder
-    result = Column.Value (Java_Column.new effective_new_name folded)
-    Problem_Behavior.Report_Warning.attach_problems_after result <|
-        Java_Problems.parse_aggregated_problems problem_builder.getProblems
+    common_type_check = case check_common_type of
+        True  -> type_helpers.find_common_type_for_arguments effective_operands
+        False -> Nothing
+    common_type_check.if_not_error <|
+        problem_builder = MapOperationProblemBuilder.new effective_new_name
+        folded = effective_operands.fold column.java_column.getStorage current-> operand->
+            case operand of
+                _ : Column ->
+                    current.zip name fallback_fn operand.java_column.getStorage skip_nulls problem_builder
+                _ ->
+                    current.bimap name fallback_fn operand skip_nulls problem_builder
+        result = Column.Value (Java_Column.new effective_new_name folded)
+        Problem_Behavior.Report_Warning.attach_problems_after result <|
+            Java_Problems.parse_aggregated_problems problem_builder.getProblems
 
 ## PRIVATE
 

diff --git a/distribution/lib/Standard/Table/0.0.0-dev/src/Data/Type/Value_Type_Helpers.enso b/distribution/lib/Standard/Table/0.0.0-dev/src/Data/Type/Value_Type_Helpers.enso
@@ -157,6 +157,14 @@ type Operation_Type_Helpers
                 Invalid_Value_Type.Error expected_type (self.find_argument_type argument) related_column=Nothing
         Error.throw error
 
+    ## PRIVATE
+    find_common_type_for_arguments self arguments =
+        types = arguments.map self.find_argument_type
+        case find_common_type types strict=True of
+            common_type : Value_Type -> common_type
+            Nothing -> Error.throw <|
+                No_Common_Type.Error types related_column_name=Nothing
+
 
 ## PRIVATE
    Type helpers for in-memory tables.

diff --git a/distribution/lib/Standard/Table/0.0.0-dev/src/Errors.enso b/distribution/lib/Standard/Table/0.0.0-dev/src/Errors.enso
@@ -451,16 +451,26 @@ type Column_Type_Mismatch
 
 type No_Common_Type
     ## PRIVATE
-       An error indicating that no common type could be found for the merged
-       columns.
-    Error (column_name : Text)
+       An error indicating that no common type could be found.
+
+       Arguments:
+       - types: The types that were tried to be unified.
+       - related_column_name: The name of the resulting column that was being
+         unified, if applicable.
+    Error (types : Vector Value_Type) (related_column_name : Nothing|Text)
 
     ## PRIVATE
 
        Create a human-readable version of the error.
     to_display_text : Text
     to_display_text self =
-        "No common type could have been found for the columns corresponding to ["+self.column_name+"]. If you want to allow mixed types, please retype the columns to the `Mixed` before the concatenation (note however that most Database backends do not support `Mixed` types, so it may work only for the in-memory backend)."
+        types = self.types.map .to_display_text . join ", "
+        prefix = "No common type could have been found for the types: "+types
+        infix = case self.related_column_name of
+            Nothing -> "."
+            column_name : Text -> " when unifying the column ["+column_name+"]."
+        suffix = "If you want to allow mixed types, please retype the columns to the `Mixed` before the concatenation (note however that most Database backends do not support `Mixed` types, so it may work only for the in-memory backend)."
+        prefix + infix + suffix
 
 type Unmatched_Columns
     ## PRIVATE

diff --git a/std-bits/base/src/main/java/org/enso/base/polyglot/NumericConverter.java b/std-bits/base/src/main/java/org/enso/base/polyglot/NumericConverter.java
@@ -42,7 +42,7 @@ public static long coerceToLong(Object o) {
       case Integer x -> x.longValue();
       case Short x -> x.longValue();
       case Byte x -> x.longValue();
-      default -> throw new UnsupportedOperationException();
+      default -> throw new UnsupportedOperationException("Cannot coerce " + o + " to a numeric type.");
     };
   }
 

diff --git a/std-bits/table/src/main/java/org/enso/table/data/column/builder/object/BoolBuilder.java b/std-bits/table/src/main/java/org/enso/table/data/column/builder/object/BoolBuilder.java
@@ -29,8 +29,12 @@ public void appendNoGrow(Object o) {
     if (o == null) {
       isNa.set(size);
     } else {
-      if ((Boolean) o) {
-        vals.set(size);
+      if (o instanceof Boolean b) {
+        if (b) {
+          vals.set(size);
+        }
+      } else {
+        throw new UnsupportedOperationException("Cannot coerce " + o + " to a boolean type.");
       }
     }
     size++;

diff --git a/std-bits/table/src/main/java/org/enso/table/data/column/storage/BoolStorage.java b/std-bits/table/src/main/java/org/enso/table/data/column/storage/BoolStorage.java
@@ -180,10 +180,10 @@ public boolean isNegated() {
     return negated;
   }
 
-  public Storage<?> iif(Value when_true, Value when_false) {
+  public Storage<?> iif(Value when_true, Value when_false, StorageType resultStorageType) {
     var on_true = makeRowProvider(when_true);
     var on_false = makeRowProvider(when_false);
-    InferredBuilder builder = new InferredBuilder(size);
+    Builder builder = Builder.getForType(resultStorageType, size);
     for (int i = 0; i < size; i++) {
       if (isMissing.get(i)) {
         builder.append(null);

diff --git a/test/Table_Tests/src/Common_Table_Operations/Column_Operations_Spec.enso b/test/Table_Tests/src/Common_Table_Operations/Column_Operations_Spec.enso
@@ -28,13 +28,8 @@ spec setup =
             c2.to_vector . should_equal [22, 33, Nothing, 22]
             c2.value_type . is_floating_point . should_be_true
 
-            c3 = t.at "X" . iif 22.0 False
-            case setup.test_selection.booleans_automatically_coerced_to_ints of
-                True ->
-                    c3.to_vector . should_equal [22, 0, Nothing, 22]
-                    c3.value_type . is_floating_point . should_be_true
-                False ->
-                    c3.to_vector . should_fail_with SQL_Error
+            t.at "X" . iif 22.0 False . to_vector . should_fail_with No_Common_Type
+            t.at "X" . iif 22 "0" . to_vector . should_fail_with No_Common_Type
 
         Test.specify "iif on Columns" <|
             t1 = table_builder [["X", [True, False, Nothing, False]], ["Y", [1, 2, 3, 4]], ["Z", [1.5, 2.5, 3.5, 4.5]]]
@@ -48,13 +43,8 @@ spec setup =
             c2.value_type . is_floating_point . should_be_true
 
             t3 = table_builder [["X", [True, False]], ["Y", [10, 20]], ["Z", [False, True]]]
-            c3 = t3.at "X" . iif (t3.at "Y") (t3.at "Z")
-            case setup.test_selection.booleans_automatically_coerced_to_ints of
-                True ->
-                    c3.to_vector . should_equal [10, 1]
-                    c3.value_type . is_integer . should_be_true
-                False ->
-                    c3.to_vector . should_fail_with SQL_Error
+            t3.at "X" . iif (t3.at "Y") (t3.at "Z") . to_vector . should_fail_with No_Common_Type
+            t3.at "X" . iif (t3.at "Y") "<NA>" . to_vector . should_fail_with No_Common_Type
 
         Test.specify "should allow to compute &&, || and not" <|
             t = table_builder [["X", [True, False, True]], ["Y", [True, False, False]]]
@@ -392,10 +382,6 @@ spec setup =
             x.fill_nothing 1 . value_type . is_integer . should_be_true
             x.fill_nothing 1.0 . value_type . is_floating_point . should_be_true
 
-            c = x.fill_nothing True
-            c.value_type.is_floating_point.should_be_true
-            c.to_vector . should_equal [1, 4, 5, 1]
-
             y.fill_nothing 1 . value_type . is_floating_point . should_be_true
             c2 = y.fill_nothing False
             c2.value_type.is_floating_point.should_be_true
@@ -405,6 +391,9 @@ spec setup =
             c3.value_type.is_floating_point.should_be_true
             c3.to_vector . should_equal [1, 4, 5, 0]
 
+            x.fill_nothing True . to_vector . should_fail_with No_Common_Type
+            y.fill_nothing True . to_vector . should_fail_with No_Common_Type
+
     Test.group prefix+"Text Column Operations" <|
         t3 = table_builder [["s1", ["foobar", "bar", "baz", "BAB", Nothing]], ["s2", ["foo", "ar", "a", "b", Nothing]]]
         s1 = t3.at "s1"

diff --git a/test/Table_Tests/src/Common_Table_Operations/Join/Union_Spec.enso b/test/Table_Tests/src/Common_Table_Operations/Join/Union_Spec.enso
@@ -250,16 +250,26 @@ spec setup =
             t1 = table_builder [["A", [1, 2, 3]], ["B", ["a", "b", "c"]], ["C", [True, False, Nothing]]]
             t2 = table_builder [["C", ["x", "Y", "Z"]], ["A", [4, 5, 6]], ["B", [1, 2, 3]]]
 
-            action = t1.union t2 on_problems=_
-            tester table =
-                expect_column_names ["A"] table
-                table.at "A" . to_vector . should_equal [1, 2, 3, 4, 5, 6]
-            problems = [No_Common_Type.Error "B", No_Common_Type.Error "C"]
-            Problems.test_problem_handling action problems tester
+            r1 = t1.union t2 on_problems=Problem_Behavior.Report_Error
+            r1.should_fail_with No_Common_Type
+
+            r2 = t1.union t2 on_problems=Problem_Behavior.Ignore
+            Problems.assume_no_problems r2
+
+            r3 = t1.union t2 on_problems=Problem_Behavior.Report_Warning
+            w3 = Problems.get_attached_warnings r3
+            w3.each w-> w.should_be_a No_Common_Type
+            w3.map w->
+                ## We look just at names of the Value_Type constructors, as
+                   different database backends may choose integers of different
+                   sizes and have differing settings for text types.
+                types = w.types.map value_type->
+                    Meta.meta value_type . constructor . name
+                (types == ["Char", "Integer"]) || (types == ["Boolean", "Char"]) . should_be_true
 
             # A boolean column cannot be merged with integers.
-            t3 = t1.select_columns ["C"] . union (t2.select_columns ["A"]) match_columns=Match_Columns.By_Position on_problems=Problem_Behavior.Report_Error
-            t3.should_fail_with No_Common_Type
+            r4 = t1.select_columns ["C"] . union (t2.select_columns ["A"]) match_columns=Match_Columns.By_Position on_problems=Problem_Behavior.Report_Error
+            r4.should_fail_with No_Common_Type
 
         Test.specify "if type widening is not allowed, should use the type from first table that contained the given column" <|
             t1 = table_builder [["A", [1, 2, 3]]]

diff --git a/test/Table_Tests/src/Common_Table_Operations/Main.enso b/test/Table_Tests/src/Common_Table_Operations/Main.enso
@@ -88,10 +88,7 @@ type Test_Selection
        - date_time: Specifies if the backend supports date/time operations.
        - fixed_length_text_columns: Specifies if the backend supports fixed
          length text columns.
-       - booleans_automatically_coerced_to_ints: Specifies if the backend
-         automatically coerces booleans to ints when performing operations on
-         them. If False, explicit casts may be needed.
-    Config supports_case_sensitive_columns=True order_by=True natural_ordering=False case_insensitive_ordering=True order_by_unicode_normalization_by_default=False case_insensitive_ascii_only=False take_drop=True allows_mixed_type_comparisons=True supports_unicode_normalization=False is_nan_and_nothing_distinct=True distinct_returns_first_row_from_group_if_ordered=True date_time=True fixed_length_text_columns=False booleans_automatically_coerced_to_ints=True
+    Config supports_case_sensitive_columns=True order_by=True natural_ordering=False case_insensitive_ordering=True order_by_unicode_normalization_by_default=False case_insensitive_ascii_only=False take_drop=True allows_mixed_type_comparisons=True supports_unicode_normalization=False is_nan_and_nothing_distinct=True distinct_returns_first_row_from_group_if_ordered=True date_time=True fixed_length_text_columns=False
 
 spec setup =
     Core_Spec.spec setup