Merge pull request #2399 from ada-x64/features/expr-alias-refactor

Expressions API Updates

packages/perspective/index.d.ts

@@ -138,7 +138,7 @@ declare module "@finos/perspective" {
     };
 
     export type ColumnName = string | null;
-    export type Expression = string;
+    export type Expression = { name: string; expr: string };
     export type Filter = [
         ColumnName,
         FilterOp,
@@ -169,7 +169,7 @@ declare module "@finos/perspective" {
         delete(): Promise<void>;
         on_delete(callback: () => void): void;
         validate_expressions(
-            expressions: Array<string>
+            expressions: Array<Expression>
         ): Promise<ValidatedExpressions>;
         schema(): Promise<Schema>;
         size(): Promise<number>;

packages/perspective/src/js/perspective.js

@@ -1415,13 +1415,13 @@ export default function (Module) {
      *  which will be used in the engine to look up column values.
      *
      * @private
-     * @param {Array<String>} expressions
+     * @param {Array<{name: string, expr: string}> | Array<string>} expressions
      */
     function parse_expression_strings(expressions) {
         let validated_expressions = [];
         const expression_idx_map = {};
 
-        for (let expression_string of expressions) {
+        for (let expression of expressions) {
             // Map of column names to column IDs, so that we generate
             // column IDs correctly without collision.
             let column_name_map = {};
@@ -1434,19 +1434,24 @@ export default function (Module) {
             // First, look for a column alias, which is a // style comment
             // on the first line of the expression.
             let expression_alias;
-            let alias_match = expression_string.match(/^\/\/(?<alias>.+?)\n/);
+            if (expression.name && expression.name !== "") {
+                expression_alias = expression.name;
+            } else {
+                let alias_match = expression.match(/^\/\/(?<alias>.+?)\n/);
 
-            if (alias_match?.groups?.alias) {
-                expression_alias = alias_match.groups.alias.trim();
-            }
+                if (alias_match?.groups?.alias) {
+                    expression_alias = alias_match.groups.alias.trim();
+                }
 
-            // If an alias does not exist, the alias is the expression itself.
-            if (!expression_alias || expression_alias.length == 0) {
-                expression_alias = expression_string;
+                // If an alias does not exist, the alias is the expression itself.
+                if (!expression_alias || expression_alias.length == 0) {
+                    expression_alias = expression;
+                }
             }
 
             // Replace `true` and `false` reserved words with symbols
-            let parsed_expression_string = expression_string.replace(
+            let expr_val = expression.expr ?? expression;
+            let parsed_expression_string = expr_val.replace(
                 /([a-zA-Z_]+[a-zA-Z0-9_]*)/g,
                 (match) => {
                     if (match == "true") {
@@ -1516,7 +1521,7 @@ export default function (Module) {
 
             const validated = [
                 expression_alias,
-                expression_string,
+                expr_val,
                 parsed_expression_string,
                 column_id_map,
             ];
@@ -1544,7 +1549,7 @@ export default function (Module) {
      * passed in is not in `expressions`, it is guaranteed to be in `errors`.
      *
      * @async
-     * @param {Array<String>} expressions An array of string expressions to
+     * @param {Array<{name: string, expr: string}>} expressions An array of expressions to
      * be validated.
      *
      * @returns {Promise<Object>}

packages/perspective/test/js/expressions/functionality.spec.js

@@ -3253,6 +3253,46 @@ const perspective = require("@finos/perspective");
                 view.delete();
                 table.delete();
             });
+
+            test("Validation works with both input formats", async function () {
+                const table = await perspective.table({
+                    a: [1, 2, 3, 4],
+                    b: [1, 2, 3, 4],
+                });
+                const str_exprs = [
+                    '// x\n"a"',
+                    '// y\n"b" * 0.5',
+                    "// c\n'abcdefg'",
+                    "// d\ntrue and false",
+                    '// e\nfloat("a") > 2 ? null : 1',
+                    "// f\ntoday()",
+                    "// g\nnow()",
+                    "// h\nlength(123)",
+                ];
+                const dict_exprs = [
+                    { name: "x", expr: '"a"' },
+                    { name: "y", expr: '"b" * 0.5' },
+                    { name: "c", expr: "'abcdefg'" },
+                    { name: "d", expr: "true and false" },
+                    { name: "e", expr: 'float("a") > 2 ? null : 1' },
+                    { name: "f", expr: "today()" },
+                    { name: "g", expr: "now()" },
+                    { name: "h", expr: "length(123)" },
+                ];
+                let vstrs = await table.validate_expressions(str_exprs);
+                let vdicts = await table.validate_expressions(dict_exprs);
+
+                expect(vstrs.errors).toEqual(vdicts.errors);
+                expect(vstrs.expression_schema).toEqual(
+                    vdicts.expression_schema
+                );
+
+                for (let [key, val] of Object.entries(vstrs.expression_alias)) {
+                    let dict_val = vdicts.expression_alias[key];
+                    let parsed_val = `// ${key}\n${dict_val}`;
+                    expect(val).toEqual(parsed_val);
+                }
+            });
         });
     });
 })(perspective);

python/perspective/perspective/table/_utils.py

@@ -10,6 +10,7 @@
 #  ┃ of the [Apache License 2.0](https://www.apache.org/licenses/LICENSE-2.0). ┃
 #  ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
 
+from operator import itemgetter
 import re
 from datetime import date, datetime
 from functools import partial
@@ -142,7 +143,7 @@ def _replace_interned_param(match_obj):
     return "{}'{}'{}".format(full[0:intern_idx], value, full[intern_idx + len(intern_fn) :])
 
 
-def _parse_expression_strings(expressions):
+def _parse_expression_inputs(expressions):
     """Given a list of string expressions, parse out column names and string
     literals using regex and return a list of lists that contain three items
     in each inner list:
@@ -156,24 +157,24 @@ def _parse_expression_strings(expressions):
     alias_map = {}
 
     for expression in expressions:
-        if '""' in expression:
+        if type(expression) is dict:
+            expr_raw, alias = itemgetter("expr", "name")(expression)
+            parsed = expr_raw
+        else:
+            expr_raw = expression
+            parsed = expr_raw
+            alias_match = re.match(ALIAS_REGEX, expr_raw)
+            if alias_match:
+                alias = alias_match.group(1).strip()
+            else:
+                alias = expr_raw
+
+        if '""' in expr_raw:
             raise ValueError("Cannot reference empty column in expression!")
 
         column_id_map = {}
         column_name_map = {}
 
-        alias_match = re.match(ALIAS_REGEX, expression)
-
-        # initialize `parsed` here so we keep `expression` unedited as the
-        # user typed it into Perspective
-        parsed = expression
-
-        if alias_match:
-            alias = alias_match.group(1).strip()
-        else:
-            # Expression itself is the alias
-            alias = expression
-
         # we need to be able to modify the running_cidx inside of every call to
         # replacer_fn - must pass by reference unfortunately
         running_cidx = [0]
@@ -204,7 +205,7 @@ def _parse_expression_strings(expressions):
         parsed = re.sub(FUNCTION_LITERAL_REGEX, _replace_interned_param, parsed)
         parsed = re.sub(REPLACE_FN_REGEX, _replace_interned_param, parsed)
 
-        validated = [alias, expression, parsed, column_id_map]
+        validated = [alias, expr_raw, parsed, column_id_map]
 
         if alias_map.get(alias) is not None:
             idx = alias_map[alias]

python/perspective/perspective/table/table.py

@@ -20,7 +20,7 @@
 from ._utils import (
     _dtype_to_pythontype,
     _dtype_to_str,
-    _parse_expression_strings,
+    _parse_expression_inputs,
     _str_to_pythontype,
 )
 from .libpsppy import (
@@ -196,7 +196,7 @@ def validate_expressions(self, expressions, as_string=False):
         `error_message`, `line`, and `column`.
 
         Args:
-            expressions (:obj:`list`): A list of string expressions to validate
+            expressions (:obj:`list`): A list of string expressions or {name: string, expr: string} objects to validate
                 and create a schema from.
 
         Keyword Args:
@@ -208,7 +208,7 @@ def validate_expressions(self, expressions, as_string=False):
         if len(expressions) == 0:
             return validated
 
-        expressions = _parse_expression_strings(expressions)
+        expressions = _parse_expression_inputs(expressions)
         validation_results = validate_expressions(self._table, expressions)
         expression_schema = validation_results.get_expression_schema()
         expression_errors = validation_results.get_expression_errors()
@@ -444,7 +444,7 @@ def view(
         config = {}
 
         if expressions is not None:
-            config["expressions"] = _parse_expression_strings(expressions)
+            config["expressions"] = _parse_expression_inputs(expressions)
 
         if columns is None:
             config["columns"] = self.columns()

python/perspective/perspective/tests/table/test_view_expression.py

@@ -25,6 +25,38 @@ def randstr(length, input=ascii_letters):
 
 
 class TestViewExpression(object):
+    def test_expression_conversion(self):
+        table = Table({"a": [1, 2, 3, 4], "b": [5, 6, 7, 8]})
+        test_expressions_str = [
+            '// x\n"a"',
+            '// y\n"b" * 0.5',
+            "// c\n'abcdefg'",
+            "// d\ntrue and false",
+            '// e\nfloat("a") > 2 ? null : 1',
+            "// f\ntoday()",
+            "// g\nnow()",
+            "// h\nlength(123)",
+        ]
+        test_expressions_dict = [
+            {"name": "x", "expr": '"a"'},
+            {"name": "y", "expr": '"b" * 0.5'},
+            {"name": "c", "expr": "'abcdefg'"},
+            {"name": "d", "expr": "true and false"},
+            {"name": "e", "expr": 'float("a") > 2 ? null : 1'},
+            {"name": "f", "expr": "today()"},
+            {"name": "g", "expr": "now()"},
+            {"name": "h", "expr": "length(123)"},
+        ]
+        str_validated = table.validate_expressions(test_expressions_str)
+        dict_validated = table.validate_expressions(test_expressions_dict)
+        assert str_validated["errors"] == dict_validated["errors"]
+        assert str_validated["expression_schema"] == dict_validated["expression_schema"]
+        assert str_validated["expression_alias"].keys() == dict_validated["expression_alias"].keys()
+        for dict_key, dict_val in dict_validated["expression_alias"].items():
+            str_alias = str_validated["expression_alias"]
+            matched_val = "// {}\n{}".format(dict_key, dict_val)
+            assert str_alias[dict_key] == matched_val
+
     def test_table_validate_expressions_empty(self):
         table = Table({"a": [1, 2, 3, 4], "b": [5, 6, 7, 8]})
         validate = table.validate_expressions([])

rust/perspective-viewer/src/less/column-settings-panel.less

@@ -23,4 +23,28 @@
         -webkit-mask-image: var(--column-settings-icon--mask-image);
         mask-image: var(--column-settings-icon--mask-image);
     }
+
+    // NOTE: These should probably make their way to global form styling eventually.
+    .errored {
+        outline-color: var(--error--color);
+    }
+    .item_title {
+        font-size: 9px;
+    }
+    input {
+        &[type="text"] {
+            outline: 1px solid;
+            outline-color: var(--inactive--color);
+            background-color: var(--plugin--background);
+            border: none;
+            border-radius: 2px;
+            margin-bottom: 0.5em;
+            font-family: "Roboto Mono", monospace;
+            font-size: 12px;
+            min-height: 24px;
+            &:disabled {
+                background-color: var(--inactive--color);
+            }
+        }
+    }
 }

rust/perspective-viewer/src/less/containers/tabs.less

@@ -61,9 +61,6 @@
         }
     }
     .tab-content {
-        * {
-            font-size: 12px;
-        }
         .tab-section {
             padding: 8px;
             // border-bottom: 1px solid var(--inactive--border-color);
@@ -72,8 +69,5 @@
             font-size: 14px;
             margin-left: 1em;
         }
-        .item_title {
-            margin-bottom: 1em;
-        }
     }
 }

rust/perspective-viewer/src/rust/components/column_dropdown.rs

@@ -136,7 +136,7 @@ impl Component for ColumnDropDown {
                                         <span>{ col }</span>
                                     },
                                     InPlaceColumn::Expression(col) =>  html! {
-                                        <span id="add-expression">{ col }</span>
+                                        <span id="add-expression">{ col.name.clone() }</span>
                                     },
                                 };
 

rust/perspective-viewer/src/rust/components/column_selector/active_column.rs

@@ -207,12 +207,12 @@ impl Component for ActiveColumn {
             New(InPlaceColumn::Expression(col)) => {
                 let mut view_config = ctx.props().session.get_view_config().clone();
                 if ctx.props().idx >= view_config.columns.len() {
-                    view_config.columns.push(Some(col.clone()));
+                    view_config.columns.push(Some(col.name.clone()));
                 } else {
-                    view_config.columns[ctx.props().idx] = Some(col.clone());
+                    view_config.columns[ctx.props().idx] = Some(col.name.clone());
                 }
 
-                view_config.expressions.push(col);
+                view_config.expressions.push(col.into());
                 let update = ViewConfigUpdate {
                     columns: Some(view_config.columns),
                     expressions: Some(view_config.expressions),

rust/perspective-viewer/src/rust/components/column_selector/config_selector.rs

@@ -385,8 +385,8 @@ impl Component for ConfigSelector {
             }
             ConfigSelectorMsg::New(DragTarget::GroupBy, InPlaceColumn::Expression(col)) => {
                 let mut view_config = ctx.props().session.get_view_config().clone();
-                view_config.group_by.push(col.clone());
-                view_config.expressions.push(col);
+                view_config.group_by.push(col.name.clone());
+                view_config.expressions.push(col.into());
                 let update = ViewConfigUpdate {
                     group_by: Some(view_config.group_by),
                     expressions: Some(view_config.expressions),
@@ -399,8 +399,8 @@ impl Component for ConfigSelector {
             }
             ConfigSelectorMsg::New(DragTarget::SplitBy, InPlaceColumn::Expression(col)) => {
                 let mut view_config = ctx.props().session.get_view_config().clone();
-                view_config.split_by.push(col.clone());
-                view_config.expressions.push(col);
+                view_config.split_by.push(col.name.clone());
+                view_config.expressions.push(col.into());
                 let update = ViewConfigUpdate {
                     split_by: Some(view_config.split_by),
                     expressions: Some(view_config.expressions),
@@ -414,11 +414,11 @@ impl Component for ConfigSelector {
             ConfigSelectorMsg::New(DragTarget::Filter, InPlaceColumn::Expression(col)) => {
                 let mut view_config = ctx.props().session.get_view_config().clone();
                 view_config.filter.push(Filter(
-                    col.clone(),
+                    col.name.clone(),
                     FilterOp::EQ,
                     FilterTerm::Scalar(Scalar::Null),
                 ));
-                view_config.expressions.push(col);
+                view_config.expressions.push(col.into());
                 let update = ViewConfigUpdate {
                     filter: Some(view_config.filter),
                     expressions: Some(view_config.expressions),
@@ -431,8 +431,8 @@ impl Component for ConfigSelector {
             }
             ConfigSelectorMsg::New(DragTarget::Sort, InPlaceColumn::Expression(col)) => {
                 let mut view_config = ctx.props().session.get_view_config().clone();
-                view_config.sort.push(Sort(col.clone(), SortDir::Asc));
-                view_config.expressions.push(col);
+                view_config.sort.push(Sort(col.name.clone(), SortDir::Asc));
+                view_config.expressions.push(col.into());
                 let update = ViewConfigUpdate {
                     sort: Some(view_config.sort),
                     expressions: Some(view_config.expressions),