Skip to content
This repository has been archived by the owner on Jun 4, 2024. It is now read-only.

Improve filtering type awareness #410

Merged
merged 34 commits into from
Apr 25, 2019

Conversation

Marc-Andre-Rivet
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet commented Apr 17, 2019

Follow up to #397

Feature

  • equal operator: if both lhs and rhs are numeric, converts and compares; otherwise compare lhs and rhs as-is

  • contains operator: expects a stringify-able lhs and a string rhs, returns true if the stringified lhs contains the rhs

  • like operator: expects a date string lhs and rhs, normalizes lhs and rhs, returns true if the normalized lhs starts with the normalized rhs

  • column-type based default operator
    Text, Any: contains
    Numeric: equal
    Date: like

Maintenance

  • rework the expression vs operand lexeme differentiation so that only expressions remain; the old operand is now one of the expression cases

  • refactor the terminal & validity logic of the various lexicons so they share the code instead of duplicating it

- this is for making lexemes type aware
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-410 April 17, 2019 15:50 Inactive
- fixing ts typing issues
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-410 April 17, 2019 16:18 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-410 April 17, 2019 19:10 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-410 April 17, 2019 19:33 Inactive
- "test"
- "server-test"
- "standalone-test"
- "unit-test"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were taking way too long. Breaking up the server, standalone and unit tests.


if (value && value.length) {
this.ops.set(safeColumnId, new SingleColumnSyntaxTree(safeColumnId, value));
this.ops.set(safeColumnId, new SingleColumnSyntaxTree(value, column));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now need the id and type information in the syntax tree in order to (1) apply the default field, (2) apply the right relational operator

...res.lexemes
];
}

return res;
}

export type SingleColumnConfig = RequiredPluck<IVisibleColumn, 'id'> & OptionalPluck<IVisibleColumn, 'type'>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial<IVisibleColumn> was problematic as id had to be redefined on top, causing potential misalignment issues + exposed the entire column interface for no good reason. Plucking and optionally plucking props from the type instead to create a new tailor-made partial type.

export const likeDate: IUnboundedLexeme = R.merge({
evaluate: relationalEvaluator(([op, exp]) => {
const normalizedOp = normalizeDate(op, DATE_OPTIONS);
const normalizedExp = normalizeDate(exp, DATE_OPTIONS);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to normalize content with the most flexible configuration (DATE_OPTIONS) possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, would need to know the source field + column configuration for a given evaluate -- not sure how I'd want to expose that atm

@@ -35,6 +36,7 @@ const lexicon: ILexeme[] = [
greaterThan,
lessOrEqual,
lessThan,
likeDate,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding the like operator to global, multi column and single column queries

@Marc-Andre-Rivet Marc-Andre-Rivet marked this pull request as ready for review April 25, 2019 14:19
if (rule.loader) {
rule.loader = rule.loader.replace('ts-loader', `ts-loader?${JSON.stringify({ transpileOnly: true })}`);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit hackish but it prevents TS from building up a types mapping, which is very costly in terms of memory usage.. while it's good to do so in the actual build, doing so in Cypress provides very little value and just causes a heap out of memory exception after a while.

Marc-André Rivet added 4 commits April 25, 2019 12:21
- fix terminal logic bug in lexer
- fix tests impacted by operand/expression rework
…h-table into improve-filter-type-awareness
return !R.isNil(normalizedOp) &&
!R.isNil(normalizedExp) &&
// IE11 does not support `startsWith`
normalizedOp.indexOf(normalizedExp) === 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we're worried about perf, possibly faster to avoid indexOf:

(
    normalizedOp.length === normalizedExp.length ?
    normalizedOp : normalizedOp.substr(0, normalizedExp.length)
) === normalizedExp

!R.isNil(exp) &&
!R.isNil(op) &&
(R.type(exp) === 'String' || R.type(op) === 'String') &&
op.toString().indexOf(exp.toString()) !== -1
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworked the contains logic vs. what we had previously discussed. With operand disappearing and lhs/rhs always handled with expression, it seemed unnatural to have an asymmetric contains operator that would do {a -> 1} contains "1" --> true but would do "1" contains {a -> 1} --> false.

Instead, it now makes sure that both statements are defined and that at least one is a string -- if those requirements are met, it coerces to string if necessary and does the check. The two queries above will now be true.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful. I'm not going to pretend to understand all the TS jiujitsu here, if it works I'll just watch and learn 🔬 And my one perf comment is non-🚫 - speculative anyway. 💃

@Marc-Andre-Rivet Marc-Andre-Rivet merged commit e3a266c into master Apr 25, 2019
@Marc-Andre-Rivet Marc-Andre-Rivet deleted the improve-filter-type-awareness branch April 25, 2019 23:38
@alexcjohnson alexcjohnson mentioned this pull request Apr 30, 2019
4 tasks
@chriddyp chriddyp mentioned this pull request Jun 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants