Merge pull request #1441 from finos/expression-errors

Output more metadata on expression errors, fix #1440

azure-pipelines.yml

@@ -194,25 +194,25 @@ jobs:
       condition: failed()
       inputs:
         targetPath: '$(System.DefaultWorkingDirectory)/packages/perspective-viewer/screenshots/'
-        artifactName: "perspective-viewer"
+        artifactName: "perspective-viewer-$(Build.BuildNumber)"
 
     - task: PublishPipelineArtifact@1
       condition: failed()
       inputs:
         targetPath: '$(System.DefaultWorkingDirectory)/packages/perspective-viewer-datagrid/screenshots/'
-        artifactName: "perspective-viewer-datagrid"
+        artifactName: "perspective-viewer-datagrid-$(Build.BuildNumber)"
 
     - task: PublishPipelineArtifact@1
       condition: failed()
       inputs:
         targetPath: '$(System.DefaultWorkingDirectory)/packages/perspective-viewer-d3fc/screenshots/'
-        artifactName: "perspective-viewer-d3fc"
+        artifactName: "perspective-viewer-d3fc-$(Build.BuildNumber)"
 
     - task: PublishPipelineArtifact@1
       condition: failed()
       inputs:
         targetPath: '$(System.DefaultWorkingDirectory)/packages/perspective-workspace/screenshots/'
-        artifactName: "perspective-workspace"
+        artifactName: "perspective-workspace-$(Build.BuildNumber)"
 
 
 - job: 'Linux'

cpp/perspective/src/cpp/computed_expression.cpp

@@ -318,7 +318,7 @@ t_computed_expression_parser::get_dtype(
     const std::string& parsed_expression_string,
     const std::vector<std::pair<std::string, std::string>>& column_ids,
     const t_schema& schema,
-    std::string& error_string
+    t_expression_error& error
 ) {
     exprtk::symbol_table<t_tscalar> sym_table;
     sym_table.add_constants();
@@ -337,7 +337,9 @@ t_computed_expression_parser::get_dtype(
         const std::string& column_name = column_ids[cidx].second;
 
         if (!schema.has_column(column_name)) {
-            error_string += ("Value Error - Input column \"" + column_name + "\" does not exist.");
+            error.m_error_message = ("Value Error - Input column \"" + column_name + "\" does not exist.");
+            error.m_line = 0;
+            error.m_column = 0;
             return DTYPE_NONE;
         }
 
@@ -353,55 +355,90 @@ t_computed_expression_parser::get_dtype(
     expr_definition.register_symbol_table(sym_table);
 
     if (!t_computed_expression_parser::PARSER->compile(parsed_expression_string, expr_definition)) {
-        auto error = t_computed_expression_parser::PARSER->error();
-        // strip the Exprtk error codes such as "ERR001 -"
-        error_string += "Parser Error " + error.substr(error.find("- "));
+        // Error count should always be above 0 if there is a compile error -
+        // We simply take the first error and return it.
+        if (t_computed_expression_parser::PARSER->error_count() > 0) {
+            auto parser_error = t_computed_expression_parser::PARSER->get_error(0);
+
+            // Given an error object and an expression, `update_error` maps the
+            // error to a line and column number inside the expression. 
+            exprtk::parser_error::update_error(parser_error, parsed_expression_string);
+
+            // Take the error message and strip the ExprTk error code
+            std::string error_message(parser_error.diagnostic.c_str());
+
+            // strip the Exprtk error codes such as "ERR001 -"
+            error.m_error_message = "Parser Error " + error_message.substr(error_message.find("- "));
+
+            error.m_line = parser_error.line_no;
+            error.m_column = parser_error.column_no;
+        } else {
+            // If for whatever reason the error count is at 0, output a generic
+            // parser error so that we report back a valid error object.
+            error.m_error_message = "Parser Error";
+            error.m_line = 0;
+            error.m_column = 0;
+        }
+
         return DTYPE_NONE;
     }
 
     t_tscalar v = expr_definition.value();
 
     if (v.m_status == STATUS_CLEAR) {
-        error_string += "Type Error - inputs do not resolve to a valid expression.";
+        error.m_error_message = "Type Error - inputs do not resolve to a valid expression.";
+        error.m_line = 0;
+        error.m_column = 0;
         return DTYPE_NONE;
     }
 
     return v.get_dtype();
 }
 
-t_validated_expression_map::t_validated_expression_map(t_uindex capacity) {
-    if (capacity > 0) {
-        m_expressions.reserve(capacity);
-        m_results.reserve(capacity);
-        m_expression_map.reserve(capacity);
-    }
-}
+t_validated_expression_map::t_validated_expression_map() {}
 
 void
-t_validated_expression_map::add(
-    const std::string& expression_alias, const std::string& result) {
-    if (m_expression_map.count(expression_alias)) {
-        t_uindex idx = m_expression_map[expression_alias];
-        m_results[idx] = result;
-    } else {
-        m_expressions.push_back(expression_alias);
-        m_results.push_back(result);
-        m_expression_map[expression_alias] = m_expressions.size() - 1;
+t_validated_expression_map::add_expression(
+    const std::string& expression_alias, const std::string& type_string) {
+    // If the expression is in the error map, it should be removed as it is now
+    // valid. This can happen when validating multiple expressions with the same
+    // alias where the first instance is invalid, and the next occurence is
+    // valid, and so on. This maintains the property that an expression will
+    // exist in the schema OR the error map.
+    auto error_iter = m_expression_errors.find(expression_alias);
+
+    if (error_iter != m_expression_errors.end()) {
+        m_expression_errors.erase(error_iter);
     }
+
+    m_expression_schema[expression_alias] = type_string;
 }
 
-t_uindex
-t_validated_expression_map::size() const {
-    return m_expression_map.size();
+void 
+t_validated_expression_map::add_error(
+    const std::string& expression_alias, t_expression_error& error) {
+    // If the expression is in the schema, it should be removed as it is now
+    // invalid. This can happen when validating multiple expressions with the
+    // same alias where the first instance is valid, and the next occurence is
+    // invalid, and so on. This maintains the property that an expression will
+    // exist in the schema OR the error map.
+    auto schema_iter = m_expression_schema.find(expression_alias);
+
+    if (schema_iter != m_expression_schema.end()) {
+        m_expression_schema.erase(schema_iter);
+    }
+
+    m_expression_errors[expression_alias] = error;
 }
 
-const std::vector<std::string>&
-t_validated_expression_map::get_expressions() const {
-    return m_expressions;
+std::map<std::string, std::string>
+t_validated_expression_map::get_expression_schema() const {
+    return m_expression_schema;
 }
 
-const std::vector<std::string>&
-t_validated_expression_map::get_results() const {
-    return m_results;
+std::map<std::string, t_expression_error>
+t_validated_expression_map::get_expression_errors() const {
+    return m_expression_errors;
 }
+
 } // end namespace perspective

cpp/perspective/src/cpp/emscripten.cpp

@@ -2030,20 +2030,29 @@ EMSCRIPTEN_BINDINGS(perspective) {
         .function("_process", &t_pool::_process)
         .function("set_update_delegate", &t_pool::set_update_delegate);
 
+    /******************************************************************************
+     *
+     * t_tscalar
+     */
+    class_<t_tscalar>("t_tscalar");
+
     /******************************************************************************
      *
      * t_validated_expression_map
      */
     class_<t_validated_expression_map>("t_validated_expression_map")
-        .constructor<t_uindex>()
-        .function("get_expressions", &t_validated_expression_map::get_expressions)
-        .function("get_results", &t_validated_expression_map::get_results);
+        .constructor<>()
+        .function("get_expression_schema", &t_validated_expression_map::get_expression_schema)
+        .function("get_expression_errors", &t_validated_expression_map::get_expression_errors);
 
     /******************************************************************************
      *
-     * t_tscalar
+     * t_expression_error
      */
-    class_<t_tscalar>("t_tscalar");
+    value_object<t_expression_error>("t_expression_error")
+        .field("error_message", &t_expression_error::m_error_message)
+        .field("line", &t_expression_error::m_line)
+        .field("column", &t_expression_error::m_column);
 
     /******************************************************************************
      *
@@ -2095,9 +2104,13 @@ EMSCRIPTEN_BINDINGS(perspective) {
      */
     register_map<std::string, std::string>(
         "std::map<std::string, std::string>");
+
     register_map<std::string, std::map<std::string, std::string>>(
         "std::map<std::string, std::map<std::string, std::string>>");
 
+    register_map<std::string, t_expression_error>(
+        "std::map<std::string, t_expression_error>");
+
     /******************************************************************************
      *
      * t_dtype

cpp/perspective/src/cpp/table.cpp

@@ -73,7 +73,7 @@ Table::validate_expressions(
             std::string,
             std::string,
             std::vector<std::pair<std::string, std::string>>>>& expressions) const {
-    t_validated_expression_map rval(expressions.size());
+    t_validated_expression_map rval = t_validated_expression_map();
 
     // Expression columns live on the `t_gstate` master table, so this
     // schema will always contain ALL expressions columns created by ALL views
@@ -88,45 +88,35 @@ Table::validate_expressions(
 
     for (const auto& expr : expressions) {
         const std::string& expression_alias = std::get<0>(expr);
-        std::string error_string;
+        const std::string& expression_string = std::get<1>(expr);
+        const std::string& parsed_expression_string = std::get<2>(expr);
+        t_expression_error error;
 
         // Cannot overwrite a "real" column with an expression column
         if (gnode_schema.has_column(expression_alias)) {
-            error_string += "Value Error - expression \"" + expression_alias + "\" cannot overwrite an existing column.";
-            rval.add(expression_alias, error_string);
+            error.m_error_message = "Value Error - expression \"" + expression_alias + "\" cannot overwrite an existing column.";
+            error.m_line = 0;
+            error.m_column = 0;
+            rval.add_error(expression_alias, error);
             continue;
         }
 
+        const auto& column_ids = std::get<3>(expr);
+
         t_dtype expression_dtype = t_computed_expression_parser::get_dtype(
             expression_alias,
-            std::get<1>(expr),
-            std::get<2>(expr),
-            std::get<3>(expr),
+            expression_string,
+            parsed_expression_string,
+            column_ids,
             gnode_schema,
-            error_string);
+            error);
 
         if (expression_dtype == DTYPE_NONE) {
             // extract the error from the stream and set it in the returned map
-            rval.add(expression_alias, error_string);
+            rval.add_error(expression_alias, error);
         } else {
-            // Check if the expression tries to overwrite an existing
-            // expression with a different type - i.e. expression abc exists
-            // and is a float, but is being overwritten with a string. Because
-            // this causes issues with writing to columns, we should not
-            // allow this case.
-            if (master_table_schema.has_column(expression_alias) && master_table_schema.get_dtype(expression_alias) != expression_dtype) {
-                std::stringstream ss;
-                ss <<  "ValueError: cannot overwrite "
-                    << "expression \"" << expression_alias << "\" of type \""
-                    << dtype_to_str(master_table_schema.get_dtype(expression_alias))
-                    << "\" with an expression of type \""
-                    << dtype_to_str(expression_dtype)
-                    << "\"";
-                rval.add(expression_alias, ss.str());
-                continue;
-            }
-
-            rval.add(expression_alias, dtype_to_str(expression_dtype));
+            rval.add_expression(
+                expression_alias, dtype_to_str(expression_dtype));
         }
     }
 

cpp/perspective/src/include/perspective/binding.h

@@ -321,13 +321,12 @@ namespace binding {
 
     /**
      * @brief Given a table and a list of expressions, validate the expressions
-     * and return a map of all expressions with a string dtype or a string
-     * error message if the expression is invalid in any way.
+     * and return a `t_validated_expression_map` object.
      * 
      * @tparam T 
      * @param table 
      * @param j_expressions 
-     * @return t_schema 
+     * @return t_validated_expression_map 
      */
     template <typename T>
     t_validated_expression_map

cpp/perspective/src/include/perspective/computed_expression.h

@@ -26,6 +26,16 @@
 
 namespace perspective {
 
+/**
+ * @brief A thin container for an error message and position (start and end
+ * row/column) from Exprtk.
+ */
+struct PERSPECTIVE_EXPORT t_expression_error {
+    std::string m_error_message;
+    t_uindex m_line;
+    t_uindex m_column;
+};
+
 /**
  * @brief Contains the metadata for a single expression and the methods which
  * will compute the expression's output.
@@ -116,7 +126,7 @@ class PERSPECTIVE_EXPORT t_computed_expression_parser {
         const std::string& parsed_expression_string,
         const std::vector<std::pair<std::string, std::string>>& column_ids,
         const t_schema& schema,
-        std::string& error_string);
+        t_expression_error& error);
 
     static std::shared_ptr<exprtk::parser<t_tscalar>> PARSER;
 
@@ -148,25 +158,16 @@ class PERSPECTIVE_EXPORT t_computed_expression_parser {
  * offers fast lookups.
  */
 struct PERSPECTIVE_EXPORT t_validated_expression_map {
-    t_validated_expression_map(t_uindex capacity);
+    t_validated_expression_map();
 
-    /**
-     * @brief Add an expression and its result (a `t_dtype` as string or an
-     * error message) to the map. If the expression is already in the map,
-     * it is replaced with the new result.
-     * 
-     * @param expression 
-     * @param result 
-     */
-    void add(const std::string& expression_alias, const std::string& result);
-
-    t_uindex size() const;
-    const std::vector<std::string>& get_expressions() const;
-    const std::vector<std::string>& get_results() const;
-
-    std::vector<std::string> m_expressions;
-    std::vector<std::string> m_results;
-    tsl::hopscotch_map<std::string, t_uindex> m_expression_map;
+    void add_expression(const std::string& expression_alias, const std::string& type_string);
+    void add_error(const std::string& expression_alias, t_expression_error& error);
+
+    std::map<std::string, std::string> get_expression_schema() const;
+    std::map<std::string, t_expression_error> get_expression_errors() const;
+
+    std::map<std::string, std::string> m_expression_schema;
+    std::map<std::string, t_expression_error> m_expression_errors;
 };
 
 } // end namespace perspective