Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expressions API Updates #2399

Merged
merged 2 commits into from
Nov 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/perspective/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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>;
Expand Down
29 changes: 17 additions & 12 deletions packages/perspective/src/js/perspective.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {};
Expand All @@ -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") {
Expand Down Expand Up @@ -1516,7 +1521,7 @@ export default function (Module) {

const validated = [
expression_alias,
expression_string,
expr_val,
parsed_expression_string,
column_id_map,
];
Expand Down Expand Up @@ -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>}
Expand Down
40 changes: 40 additions & 0 deletions packages/perspective/test/js/expressions/functionality.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
31 changes: 16 additions & 15 deletions python/perspective/perspective/table/_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand All @@ -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]
Expand Down Expand Up @@ -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]
Expand Down
8 changes: 4 additions & 4 deletions python/perspective/perspective/table/table.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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:
Expand All @@ -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()
Expand Down Expand Up @@ -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()
Expand Down
32 changes: 32 additions & 0 deletions python/perspective/perspective/tests/table/test_view_expression.py
Original file line number Diff line number Diff line change
Expand Up @@ -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([])
Expand Down
24 changes: 24 additions & 0 deletions rust/perspective-viewer/src/less/column-settings-panel.less
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
}
6 changes: 0 additions & 6 deletions rust/perspective-viewer/src/less/containers/tabs.less
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,6 @@
}
}
.tab-content {
* {
font-size: 12px;
}
.tab-section {
padding: 8px;
// border-bottom: 1px solid var(--inactive--border-color);
Expand All @@ -72,8 +69,5 @@
font-size: 14px;
margin-left: 1em;
}
.item_title {
margin-bottom: 1em;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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>
},
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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),
Expand All @@ -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),
Expand All @@ -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),
Expand Down
Loading
Loading