Issue 545 - Case (in)sensitive filtering by Marc-Andre-Rivet · Pull Request #893 · plotly/dash-table

Marc-Andre-Rivet · 2021-04-29T00:35:47Z

Closes #545. Uses as as a starting point and supersedes #609.

@wbrgss What follows is not necessary for the review itself but, if you want to dig a little bit into the table's query parser / AST implementation, here's the key things to look at:

the lexemes are defined here: https://github.com/plotly/dash-table/tree/dev/src/dash-table/syntax-tree/lexeme (these represent all the lexical units of the table's query language; these are "unbounded" as in not yet part of a vocabulary with allowed relationships

dash-table/src/core/syntax-tree/lexicon.ts

Line 13 in 8172683

export interface IUnboundedLexeme {

)
the various flavors of lexicons are defined here: https://github.com/plotly/dash-table/tree/dev/src/dash-table/syntax-tree/lexicon (including the full query language supported by the table and a small special subset used for the column queries; notice here that the lexemes here become "bounded" by a set of rules that define usage

dash-table/src/core/syntax-tree/lexicon.ts

Line 27 in 8172683

export interface ILexeme extends IUnboundedLexeme {

)
the Lexer (technically a mix of parser / tokenizer / lexer) that takes the input string alongside the lexicon to output lexemes

dash-table/src/core/syntax-tree/lexer.ts

Line 16 in 8172683

export default function lexer(lexicon: Lexicon, query: string): ILexerResult {
the Syntaxer that takes the lexemes output from the Lexer and builds the AST

dash-table/src/core/syntax-tree/syntaxer.ts

Line 76 in 8172683

return {tree: parser(lexemes), valid: true};

Marc-Andre-Rivet · 2021-04-29T00:38:44Z

+        const match = query.match(next.regexp) ?? [];
+        const value = match[next.regexpMatch || 0];
+        const flags = match[next.regexpFlags || -1];
+        result.push({lexeme: next, flags, value});


From a lexicon perspective treat i|s prefix to applicable relational operators as "flags" that can be used to determine the exact processing to do during evaluation. value is the "base" operator + flags (e.g. icontains)

Marc-Andre-Rivet · 2021-04-29T00:39:27Z

+                <FilterOptions
+                    filterOptions={filterOptions}
+                    toggleFilterOptions={toggleFilterOptions}
+                />


New component displayed in the filter cell to toggle filtering options

Marc-Andre-Rivet · 2021-04-29T00:41:52Z

    clearable?: boolean | boolean[] | 'first' | 'last';
    deletable?: boolean | boolean[] | 'first' | 'last';
    editable: boolean;
+    filter_options: FilterCase;


FilterCase vs. filter_options may look inconsistent but is intended to be that way. In the future you may also want to add locale-aware comparisons and leaving some space to do so at a future time.

Marc-Andre-Rivet · 2021-04-29T00:43:21Z

+    ) => {
+        const newColumn = toggleFilterOptions(column);
+
+        updateColumnFilter(map, newColumn, operator, value, setFilter);


Trigger a setProps and return the new column configuration, use the new column configuration to update the column's query (and trigger a refresh)

Marc-Andre-Rivet · 2021-04-29T00:45:05Z

    );

+    const toggleFilterOptions = memoizeOne(
+        (setProps: SetProps, columns: IColumn[]) => (column: IColumn) => {


Called by https://github.com/plotly/dash-table/pull/893/files#r622661715

Defined at the factory level so that the individual components remain unaware of columns for memoization/cache busting / performance reasons and setProps so they remain unaware of Dash's idiosyncracies.

Marc-Andre-Rivet · 2021-04-29T00:45:47Z

+
+            setProps({columns: newColumns});
+
+            return newColumn;


update the table's columns in the shallowest way possible (minimize cache busting)

Marc-Andre-Rivet · 2021-04-29T00:47:51Z

+): boolean =>
+    relOp[0] == 'i'
+        ? fn(lhs.toString().toUpperCase(), rhs.toString().toUpperCase())
+        : fn(lhs, rhs);


Conditional evaluation of contains, =, !=, >=, >, <=, < based on the flags

Marc-Andre-Rivet · 2021-04-29T00:48:15Z

        ),
        subType: RelationalOperator.Contains,
-        regexp: /^((contains)(?=\s|$))/i,
+        regexp: /^((i|s)?contains)(?=\s|$)/i,


allow for optional flags

Marc-Andre-Rivet · 2021-04-29T00:48:53Z

        subType: RelationalOperator.Contains,
-        regexp: /^((contains)(?=\s|$))/i,
+        regexp: /^((i|s)?contains)(?=\s|$)/i,
+        regexpFlags: 2,


regex match group for usage in https://github.com/plotly/dash-table/pull/893/files#r622660202

Marc-Andre-Rivet · 2021-04-29T00:49:07Z

        ),
        subType: RelationalOperator.Equal,
-        regexp: /^(=|(eq)(?=\s|$))/i,
+        regexp: /^((i|s)?(=|(eq)(?=\s|$)))/i,


same thing for all applicable relational operators

Marc-Andre-Rivet · 2021-04-29T00:49:49Z

+                                query: `${c.hideOperand ? '' : '{a} '}seq ABC`,
+                                target: {a: 'abc'},
+                                valid: true,
+                                evaluate: false


Multiple new tests for the (in)sensitive versions of the operators -- as used by the table itself

Marc-Andre-Rivet · 2021-04-29T00:50:29Z

+            expect(tree.isValid).to.equal(true);
+            expect(tree.evaluate({a: 'abc'})).to.equal(true);
+            expect(tree.evaluate({a: 'ABC'})).to.equal(false);
+        });


Test all new operators with the various query flavors

…` toggle)

wbrgss

Looking nice so far! I'll probably do another more fine-grained pass of the code later. The functionality looks good.

If you have time @alexcjohnson I'd welcome a review from you as well

wbrgss

@Marc-Andre-Rivet Let's say I set a column to be case-insensitive using the toggle, or with column-level 'filter_options': 'insensitive'. I know I can force case-insensitive comparison using e.g. ilt, but I might also expect lt to behave the same as ilt, considering the case-insensitive toggle is on, and right now it doesn't on my end. Does that make sense?

Marc-Andre-Rivet · 2021-05-05T20:58:24Z

@wbrgss Good point. Yes it makes total sense to have lt like ilt if the toggle is turned on. I'll see about adding that in.

wbrgss

Very nice @Marc-Andre-Rivet

💃

alexcjohnson · 2021-05-11T02:17:23Z

+     * If the column-level `filter_options` prop is set it overrides
+     * the table-level `filter_options` prop for that column.
+     */
+    filter_options: PropTypes.oneOf(['sensitive', 'insensitive']),


After a bit of a slack convo with @Marc-Andre-Rivet I think we concluded we should change this to filter_options: {case: ('sensitive'|'insensitive')}. The reason he wrote it as filter_options instead of something more specific like filter_case was to allow extending to ignoring other distinctions, like accents and punctuation. But that means the current syntax is confusing as it doesn't tell you what you're controlling the sensitivity to, and to extend later we'll have to support both these strings and objects like {case: 'sensitive', accents: 'insensitive', punctuation: 'sensitive'}

The alternative would be to make a single specific prop now, like filter_case: 'sensitive', and then add more props later filter_accents: 'sensitive', filter_punctuation: 'sensitive'. That's a little easier for the most common use of enabling case-insensitive filters, but will ultimately create more props than it would need to, at both the table and column levels.

Changing this to an object works for me. I knew @Marc-Andre-Rivet wrote this with extensibility in mind and I agree this more cleanly allows for various filter-option combinations

ada8456, 6f012c3, 4697136 (mistakes were made..)

…tring to improve forward compatibility and consistency

- invert column and table filter_options priority (column takes precedence)

alexcjohnson

💃

AnnMarieW · 2021-05-12T22:10:28Z

This is so awesome! 🎊

Marc-André Rivet added 4 commits April 27, 2021 14:43

sensitive/insensitive relational operators through flags

f5783de

sensitive implicit filters

ee42e93

ensure FilterOptions re-rerenders correctly

0b7897b

changelog

8d19241

Marc-Andre-Rivet requested a review from wbrgss April 29, 2021 00:36

Marc-Andre-Rivet commented Apr 29, 2021

View reviewed changes

Comment thread .eslintrc.js

Marc-Andre-Rivet commented Apr 29, 2021

View reviewed changes

Comment thread src/dash-table/components/FilterFactory.tsx

Marc-Andre-Rivet commented Apr 29, 2021

View reviewed changes

Comment thread src/dash-table/syntax-tree/SingleColumnSyntaxTree.ts

Marc-Andre-Rivet commented Apr 29, 2021

View reviewed changes

Marc-André Rivet added 4 commits April 29, 2021 20:41

lint

49773fd

fix test (make columns wider to allow clicking elsewhere than the `Aa…

99aa5a9

…` toggle)

e2e sensitivity test

d5fddb0

resolveFlag: check for null|undefined instead of undefined only

551cd3a

wbrgss reviewed Apr 30, 2021

View reviewed changes

Marc-Andre-Rivet marked this pull request as ready for review April 30, 2021 23:47

Marc-Andre-Rivet requested review from alexcjohnson and rpkyle as code owners April 30, 2021 23:47

Marc-André Rivet added 2 commits April 30, 2021 20:59

Add title to FilterOptions component

d76ae04

trigger build

c07a701

wbrgss reviewed May 5, 2021

View reviewed changes

Marc-André Rivet added 4 commits May 7, 2021 08:09

apply sensitive/insensitive on explicit relational operators

8ddf84e

test sensitive/insensitive binary relational operators

d0c744a

trigger build

5c69ae7

trigger build

e686aea

wbrgss approved these changes May 10, 2021

View reviewed changes

This was referenced May 10, 2021

Issue 545 - Case-insensitive option for filters #609

Closed

Document new filtering syntax #900

Open

alexcjohnson reviewed May 11, 2021

View reviewed changes

Marc-André Rivet added 3 commits May 11, 2021 12:07

make filter_options an object with a case property instead of a s…

ada8456

…tring to improve forward compatibility and consistency

fix props, fix tests

6f012c3

- handle None case correctly in tests

4697136

- invert column and table filter_options priority (column takes precedence)

alexcjohnson approved these changes May 11, 2021

View reviewed changes

Marc-Andre-Rivet merged commit 776484f into dev May 11, 2021

Marc-Andre-Rivet deleted the case-insensitive-redux branch May 11, 2021 18:24

Uh oh!

Uh oh!

Conversation

Marc-Andre-Rivet commented Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbrgss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wbrgss left a comment

Choose a reason for hiding this comment

Uh oh!

Marc-Andre-Rivet commented May 5, 2021

Uh oh!

wbrgss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marc-Andre-Rivet May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcjohnson left a comment

Choose a reason for hiding this comment

Uh oh!

AnnMarieW commented May 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Marc-Andre-Rivet commented Apr 29, 2021 •

edited

Loading

Marc-Andre-Rivet May 11, 2021 •

edited

Loading