Fix scalar comparisons to return bool in exprtk

finos · texodus · Sep 26, 2021 · Sep 15, 2021 · Sep 22, 2021 · Sep 22, 2021
commit 265f1f6e9685a9f311da17e6325177062b620ba8
diff --git a/cpp/perspective/src/cpp/computed_expression.cpp b/cpp/perspective/src/cpp/computed_expression.cpp
@@ -11,8 +11,22 @@
 
 namespace perspective {
 
-std::shared_ptr<exprtk::parser<t_tscalar>> t_computed_expression_parser::PARSER
-    = std::make_shared<exprtk::parser<t_tscalar>>();
+// Change ExprTk's default compilation options to only check for correctness
+// of brackets and sequences. ExprTk defaults will replace "true" and "false"
+// with 1 and 0, which we don't want. Using the tokens "true" and "false"
+// will raise a syntax error, which is the correct behavior.
+std::size_t
+t_computed_expression_parser::PARSER_COMPILE_OPTIONS = 
+    exprtk::parser<t_tscalar>::settings_t::e_joiner +
+    exprtk::parser<t_tscalar>::settings_t::e_numeric_check +
+    exprtk::parser<t_tscalar>::settings_t::e_bracket_check +
+    exprtk::parser<t_tscalar>::settings_t::e_sequence_check;
+    // exprtk::parser<t_tscalar>::settings_t::e_commutative_check;
+    // exprtk::parser<t_tscalar>::settings_t::e_strength_reduction;
+
+std::shared_ptr<exprtk::parser<t_tscalar>>
+t_computed_expression_parser::PARSER = 
+    std::make_shared<exprtk::parser<t_tscalar>>(t_computed_expression_parser::PARSER_COMPILE_OPTIONS);
 
 // Exprtk functions without any state can be initialized statically
 computed_function::bucket t_computed_expression_parser::BUCKET_FN
@@ -80,6 +94,12 @@ computed_function::to_string
     t_computed_expression_parser::TO_STRING_VALIDATOR_FN
     = computed_function::to_string(nullptr);
 
+t_tscalar
+t_computed_expression_parser::TRUE_SCALAR = mktscalar(true);
+
+t_tscalar
+t_computed_expression_parser::FALSE_SCALAR = mktscalar(false);
+
 #define REGISTER_COMPUTE_FUNCTIONS(vocab)                                      \
     computed_function::day_of_week day_of_week_fn                              \
         = computed_function::day_of_week(vocab);                               \
@@ -169,6 +189,10 @@ computed_function::to_string
     sym_table.add_function(                                                    \
         "datetime", t_computed_expression_parser::MAKE_DATETIME_FN);
 
+#define REGISTER_SCALAR_CONSTANTS()                                                         \
+    sym_table.add_constant("True", t_computed_expression_parser::TRUE_SCALAR);              \
+    sym_table.add_constant("False", t_computed_expression_parser::FALSE_SCALAR);            \
+
 /******************************************************************************
  *
  * t_computed_expression
@@ -192,10 +216,16 @@ t_computed_expression::compute(std::shared_ptr<t_data_table> source_table,
     std::shared_ptr<t_vocab> vocab) const {
     // TODO: share symtables across pre/re/compute
     exprtk::symbol_table<t_tscalar> sym_table;
+
+    // pi, infinity, etc.
     sym_table.add_constants();
 
-    REGISTER_COMPUTE_FUNCTIONS(vocab)
+    // "True" and "False" as boolean scalars
+    REGISTER_SCALAR_CONSTANTS()
 
+    // Custom functions from computed_functions.cpp
+    REGISTER_COMPUTE_FUNCTIONS(vocab)
+
     exprtk::expression<t_tscalar> expr_definition;
     std::vector<std::pair<std::string, t_tscalar>> values;
     tsl::hopscotch_map<std::string, std::shared_ptr<t_column>> columns;
@@ -301,6 +331,7 @@ t_computed_expression_parser::precompute(const std::string& expression_alias,
     exprtk::symbol_table<t_tscalar> sym_table;
     sym_table.add_constants();
 
+    REGISTER_SCALAR_CONSTANTS()
     REGISTER_VALIDATION_FUNCTIONS()
 
     std::vector<t_tscalar> values;
@@ -352,6 +383,7 @@ t_computed_expression_parser::get_dtype(const std::string& expression_alias,
 
     std::vector<t_tscalar> values;
 
+    REGISTER_SCALAR_CONSTANTS()
     REGISTER_VALIDATION_FUNCTIONS()
 
     auto num_input_columns = column_ids.size();

diff --git a/cpp/perspective/src/cpp/computed_function.cpp b/cpp/perspective/src/cpp/computed_function.cpp
@@ -1196,17 +1196,13 @@ namespace computed_function {
 
         t_tscalar rval;
         rval.clear();
-
-        // Return a float so we can use it in conditionals
-        rval.m_type = DTYPE_FLOAT64;
+        rval.m_type = DTYPE_BOOL;
 
         t_generic_type& gt = parameters[0];
         t_scalar_view temp(gt);
         val.set(temp());
 
-        // Return a double so we can use it in conditionals
-        rval.set(static_cast<double>(val.is_none() || !val.is_valid()));
-
+        rval.set(val.is_none() || !val.is_valid());
         return rval;
     }
 
@@ -1221,15 +1217,12 @@ namespace computed_function {
 
         t_tscalar rval;
         rval.clear();
-
-        // Return a float so we can use it in conditionals
-        rval.m_type = DTYPE_FLOAT64;
+        rval.m_type = DTYPE_BOOL;
 
         t_generic_type& gt = parameters[0];
         t_scalar_view temp(gt);
         val.set(temp());
-
-        rval.set(static_cast<double>(!val.is_none() && val.is_valid()));
+        rval.set(!val.is_none() && val.is_valid());
 
         return rval;
     }
@@ -1375,6 +1368,31 @@ namespace computed_function {
         return rval;
     }
 
+    to_boolean::to_boolean()
+    : exprtk::igeneric_function<t_tscalar>("T") {}
+
+    to_boolean::~to_boolean() {}
+
+    t_tscalar to_boolean::operator()(t_parameter_list parameters) {
+        t_tscalar val;
+        t_tscalar rval;
+        rval.clear();
+        rval.m_type = DTYPE_BOOL;
+
+        const t_generic_type& gt = parameters[0];
+        t_scalar_view temp(gt);
+        val.set(temp());
+
+        // handles STATUS_VALID, so no need to check separately
+        rval.set(val.as_bool());
+
+        if (!val.is_valid()) {
+            return rval;
+        }
+
+        return rval;
+    }
+
     make_date::make_date()
         : exprtk::igeneric_function<t_tscalar>("TTT") {}
 

diff --git a/cpp/perspective/src/cpp/scalar.cpp b/cpp/perspective/src/cpp/scalar.cpp
@@ -50,8 +50,11 @@ bool operator>(const std::size_t& lhs, const t_tscalar& rhs) {
  * @param value
  */
 t_tscalar::t_tscalar(int v) {
-    // set<double>() returns a scalar with DTYPE_FLOAT64.
-    this->set(static_cast<double>(v));
+    if (v == 0 || v == 1) {
+        this->set(static_cast<bool>(v));
+    } else {
+        this->set(static_cast<double>(v));
+    }
 }
 
 bool

diff --git a/cpp/perspective/src/cpp/sparse_tree.cpp b/cpp/perspective/src/cpp/sparse_tree.cpp
@@ -1231,21 +1231,17 @@ t_stree::update_agg_table(t_uindex nidx, t_agg_update_info& info,
                 old_value.set(dst->get_scalar(dst_ridx));
                 auto pkeys = get_pkeys(nidx);
 
-                new_value.set(
-                    reduce_from_gstate<std::function<t_tscalar(std::vector<t_tscalar>&)>>(
-                        gstate,
-                        expression_master_table,
-                        spec.get_dependencies()[0].name(),
-                        pkeys,
-                        [](std::vector<t_tscalar>& values) {
-                            t_tscalar rval;
-                            rval.set(true);
-
-                            for (const auto& v : values) {
-                                if (!v.as_bool()) {
-                                    rval.set(false);
-                                    break;
-                                }
+                new_value.set(reduce_from_gstate<
+                    std::function<t_tscalar(std::vector<t_tscalar>&)>>(gstate,
+                    expression_master_table, spec.get_dependencies()[0].name(),
+                    pkeys, [](std::vector<t_tscalar>& values) {
+                        t_tscalar rval;
+                        rval.set(true);
+
+                        for (const auto& v : values) {
+                            if (!v.as_bool()) {
+                                rval.set(false);
+                                break;
                             }
                         }
                         return rval;

diff --git a/cpp/perspective/src/include/perspective/computed_expression.h b/cpp/perspective/src/include/perspective/computed_expression.h
@@ -126,6 +126,9 @@ class PERSPECTIVE_EXPORT t_computed_expression_parser {
 
     static std::shared_ptr<exprtk::parser<t_tscalar>> PARSER;
 
+    // Applied to the parser
+    static std::size_t PARSER_COMPILE_OPTIONS;
+
     // Instances of Exprtk functions
     static computed_function::bucket BUCKET_FN;
     static computed_function::hour_of_day HOUR_OF_DAY_FN;
@@ -147,6 +150,10 @@ class PERSPECTIVE_EXPORT t_computed_expression_parser {
     static computed_function::to_float TO_FLOAT_FN;
     static computed_function::make_date MAKE_DATE_FN;
     static computed_function::make_datetime MAKE_DATETIME_FN;
+
+    // constants for True and False as DTYPE_BOOL scalars
+    static t_tscalar TRUE_SCALAR;
+    static t_tscalar FALSE_SCALAR;
 };
 
 /**

diff --git a/cpp/perspective/src/include/perspective/computed_function.h b/cpp/perspective/src/include/perspective/computed_function.h
@@ -223,6 +223,15 @@ namespace computed_function {
      */
     FUNCTION_HEADER(make_date)
 
+    /**
+     * @brief Convert a column or scalar to a boolean, which returns True if the
+     * value is truthy or False otherwise.
+     * 
+     * boolean(1)
+     * boolean(null)
+     */
+    FUNCTION_HEADER(to_boolean)
+
     /**
      * @brief Given a POSIX timestamp of milliseconds since epoch, create a
      * new datetime value.

diff --git a/cpp/perspective/src/include/perspective/exprtk.h b/cpp/perspective/src/include/perspective/exprtk.h
@@ -162,8 +162,8 @@ namespace details {
 // called, but enabling this ifdef creates a litany of compiler warnings
 // about misleading indentation inside exprtk.hpp. Considering that the UI
 // will not allow while/continue to be used, this should be ok.
-
 // #define exprtk_disable_break_continue
+
 #define exprtk_disable_return_statement
 #define exprtk_disable_rtl_io_file
 #include <exprtk.hpp>
@@ -983,8 +983,7 @@ namespace details {
 
                 // booleans need to return numeric scalars, otherwise they will
                 // not work inside a conditional for some reason.
-                rval.set(static_cast<double>(
-                    static_cast<bool>(v0) && static_cast<bool>(v1)));
+                rval.set(v0.as_bool() && v1.as_bool());
                 return rval;
             }
 
@@ -993,8 +992,7 @@ namespace details {
             or_impl(
                 const t_tscalar v0, const t_tscalar v1, t_tscalar_type_tag) {
                 t_tscalar rval;
-                rval.set(static_cast<double>(
-                    static_cast<bool>(v0) || static_cast<bool>(v1)));
+                rval.set(v0.as_bool() || v1.as_bool());
                 return rval;
             }
 
@@ -1003,7 +1001,7 @@ namespace details {
             xor_impl(
                 const t_tscalar v0, const t_tscalar v1, t_tscalar_type_tag) {
                 t_tscalar rval;
-                rval.set(static_cast<double>(!v0 != !v1));
+                rval.set(!v0.as_bool() != !v1.as_bool());
                 return rval;
             }
 
@@ -1012,8 +1010,7 @@ namespace details {
             nand_impl(
                 const t_tscalar v0, const t_tscalar v1, t_tscalar_type_tag) {
                 t_tscalar rval;
-                rval.set(static_cast<double>(
-                    !(static_cast<bool>(v0) && static_cast<bool>(v1))));
+                rval.set(!(v0.as_bool() && v1.as_bool()));
                 return rval;
             }
 
@@ -1022,8 +1019,7 @@ namespace details {
             nor_impl(
                 const t_tscalar v0, const t_tscalar v1, t_tscalar_type_tag) {
                 t_tscalar rval;
-                rval.set(static_cast<double>(
-                    !(static_cast<bool>(v0) || static_cast<bool>(v1))));
+                rval.set(!(v0.as_bool() || v1.as_bool()));
                 return rval;
             }
 
@@ -1032,8 +1028,7 @@ namespace details {
             xnor_impl(
                 const t_tscalar v0, const t_tscalar v1, t_tscalar_type_tag) {
                 t_tscalar rval;
-                rval.set(static_cast<double>(
-                    static_cast<bool>(v0) == static_cast<bool>(v1)));
+                rval.set(v0.as_bool() == v1.as_bool());
                 return rval;
             }
 
@@ -1066,7 +1061,7 @@ namespace details {
 
     inline bool
     is_true(const t_tscalar& v) {
-        return static_cast<bool>(v);
+        return v.as_bool();
     }
 
     inline bool

diff --git a/cpp/perspective/src/include/perspective/scalar.h b/cpp/perspective/src/include/perspective/scalar.h
@@ -305,6 +305,7 @@ t_tscalar::compare_common(const t_tscalar& rhs) const {
     // text-search or profiling, and changing this behavior will result in
     // a whole load of edge cases.
     if (m_type != rhs.m_type) {
+        std::cout << "comparing mixed: " << repr() << ", " << rhs.repr() << std::endl;
         COMPARER_T<unsigned char> cmp;
         return cmp(m_type, rhs.m_type);
     }

diff --git a/packages/perspective-bench/bench/versions.js b/packages/perspective-bench/bench/versions.js
@@ -10,6 +10,11 @@
 const PerspectiveBench = require("@finos/perspective-bench");
 
 const VERSIONS = [
+    "0.10.3",
+    "0.10.2",
+    "0.10.1",
+    "0.10.0",
+    "0.9.0",
     "0.8.3",
     "0.8.2",
     "0.8.1",