Skip to content

Lens formula improvements#89457

Closed
dej611 wants to merge 123 commits intoelastic:masterfrom
dej611:lens/formula-error-handling
Closed

Lens formula improvements#89457
dej611 wants to merge 123 commits intoelastic:masterfrom
dej611:lens/formula-error-handling

Conversation

@dej611
Copy link
Copy Markdown
Contributor

@dej611 dej611 commented Jan 27, 2021 •
edited
Loading

Summary

Building on top of #88997

  • Basic error handling
    • Wrong type field
    • Missing field
    • Missing operation
    • Invalid tinymath expression
    • Invalid operation first argument
    • Invalid fields math operation as argument
      • improve Error message
  • Count()
    • Make it work
  • Transferable
    • Transform previous column config into string when coming to Formula
      • Seamless execute incoming Formula
      • Workout references remapping
    • Transform Formula into config when moving away?
      • isTransferable implementation
      • Workout references remapping not required
    • Fix state clean up on dimension removal
    • Move over named arguments from previous configuration
    • Move over explicit format configuration
      • Make it work for field default formatter as well
  • Extend operations supported
    • Add moving_average, cumulative_sum, etc... (uses a name remap without the _)
    • Validate supported functions: specificOperations Use for autocomplete recommendation
  • operation parameters
    • Base positional logic for params
    • Missing argument for operation error
    • Invalid parameter passed to operation
  • column renaming
  • Add test suite to cover most validation use cases
    • Check correct validation of fn + moving_average( ... ) => does not detect missing window argument
  • Check suggestions for fullReference function when used after a first function (fn + moving_average( ...? )
  • Check suggestions for cardinality operation (why just numberic fields?)

[skip=ci]

@dej611 dej611 requested review from a team and flash1293 January 27, 2021 18:05
@dej611 dej611 requested a review from a team as a code owner January 27, 2021 18:05
@dej611 dej611 changed the title Lens formula Lens formula improments Jan 28, 2021
@dej611 dej611 changed the title Lens formula improments Lens formula improvements Jan 28, 2021
@flash1293
Copy link
Copy Markdown
Contributor

flash1293 commented Jan 28, 2021

@dej611 I tried to test the PR, but I'm only getting errors when clicking the Formula operation.

@dej611 dej611 marked this pull request as draft January 28, 2021 13:17
dej611 and others added 19 commits January 29, 2021 10:48
@dej611
Merge remote-tracking branch 'upstream/master' into lens/formula-erro…
7e0b149 
…r-handling
@dej611
🐛 Fix isTransferable from Formula
cd83321
@dej611
✨ Add all fullReferences ops + initial params
7df6c2d
@wylieconlon
Expand Tinymath grammar, including named arguments
0473cb8
@dej611
Merge remote-tracking branch 'upstream/master' into lens/formula-erro…
7897563 
…r-handling
@dej611
✨ Add enhanced validation for fields, operations and parameters
aaf5664
@wylieconlon
Add tsconfig project
66b7dde
@wylieconlon
Fix tests
0e8a7be
@dej611
✨ Add id and name (optional) decoupling
af6da20
@dej611
✨ Give Formula column custom label
7e5b825
@dej611
Merge branch 'pr/89795' into lens/formula-named-params
d6df1e2
@kibanamachine
Merge branch 'master' into tinymath-named-arguments
c558744
@wylieconlon
Allow named arguments with numeric types
bde55d3
@dej611
🏷️ Migrate AST to new format + types fix + extended validation
bf8f8c4
@dej611
🐛 Improved validation
d02ee52
@dej611
✅ Add first batch of validation tests
03b1bd4
@dej611
Merge branch 'pr/89795' into lens/formula-error-handling
3f8d248
@dej611
✅ Add more validation tests for formula
b5ee6ab
@dej611
Merge remote-tracking branch 'upstream/master' into lens/formula-erro…
9c45213 
…r-handling
wylieconlon and others added 20 commits April 5, 2021 11:03
@wylieconlon
Merge remote-tracking branch 'origin/master' into lens/formulas
ccf7266
@wylieconlon
Merge remote-tracking branch 'origin/master' into lens/formulas
0c1cfa9
@wylieconlon
Fix escape characters inside KQL
239c72d
@dej611
🐛 Fix dataType issue when moving over to Formula
fa57951
@dej611
Merge branch 'lens/formula-error-handling' of github.com:dej611/kiban…
3da8f82 
…a into lens/formula-error-handling
@wylieconlon
Automatically insert single quotes on every named param
1fd06bf
@wylieconlon
Only insert single quotes when typing kql= or lucene=
b62f96e
@wylieconlon
Reorganize help popover
d563cb5
@wylieconlon
Merge remote-tracking branch 'origin/master' into lens/formulas
32fdfbf
@wylieconlon
Merge remote-tracking branch 'origin/master' into lens/formulas
c81b47c
@wylieconlon
Fix merge issues
edb522d
@wylieconlon
Merge remote-tracking branch 'origin/master' into lens/formulas
87b435b
@wylieconlon
Update grammar for formulas
7169655
@wylieconlon
Fix bad merge
e01bca8
@wylieconlon
Rough fullscreen mode
ec493d1
@wylieconlon
Merge remote-tracking branch 'origin/master' into lens/formulas
255a40f
@wylieconlon
Type updates
90cff4a
@wylieconlon
Pass through fullscreen state
12391f6
@wylieconlon
Remove more chrome from full screen mode
83ae9bf
@wylieconlon
Fix minor bugs in formula typing
f520d2f
@flash1293
Copy link
Copy Markdown
Contributor

flash1293 commented May 4, 2021

@wylieconlon Tried to play with this but it's always failing with Unknown variable: ... for me at the moment.

@MichaelMarcialis MichaelMarcialis mentioned this pull request May 4, 2021
Merged
5 tasks
@flash1293
Copy link
Copy Markdown
Contributor

flash1293 commented May 5, 2021 •
edited
Loading

@wylieconlon Tried it again and it's still failing on these steps:

  • Drag in timestamp
  • Open count of records dimension
  • Switch to formula tab

The error message is different now though:
Screenshot 2021-05-05 at 11 54 12

It works if starting with an empty formula dimension.

I'm not entirely sure which parts I should ignore for now. Random collection of things I noticed:

  • Function documentation is incomplete and has a few layouting issues
  • Collapse fullscreen button is too large
  • Kql autocomplete doesn't autocomplete terms (on purpose)
  • I kept expecting to see the documentation for a function in the hover tooltip in the editor (probably because I'm used to it from VS code)
  • Copying a formula fails initially, but on opening the editor it starts working - maybe some routine is not kicked off correctly?
  • Probably an unrelated bug - if two series are called the same, they get pushed to different axes even if they have the same formatter configured

Screenshot 2021-05-05 at 14 12 35

* Copying an already copied formula again breaks the chart again, and this time it won't start working again on opening the editor * In some cases switching from a complex quick function config to formula works, in some cases it breaks the chart - not sure about the root cause * Scale to time unit doesn't work (might be fine for first version?)

All in all pretty usable already. This is going to be a great tool :)

@wylieconlon wylieconlon mentioned this pull request May 5, 2021
Merged
69 tasks
@wylieconlon
Copy link
Copy Markdown
Contributor

wylieconlon commented May 5, 2021

Closing in favor of #99297

@wylieconlon wylieconlon closed this May 5, 2021
@MichaelMarcialis MichaelMarcialis mentioned this pull request May 27, 2021
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@flash1293 flash1293 flash1293 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dej611 @flash1293 @kibanamachine @wylieconlon