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

Lens formula improvements #89457

Closed
wants to merge 123 commits into from
Closed

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented Jan 27, 2021

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 flash1293 and a team 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
Contributor

@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
@flash1293
Copy link
Contributor

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

@flash1293
Copy link
Contributor

flash1293 commented May 5, 2021

@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
69 tasks
@wylieconlon
Copy link
Contributor

Closing in favor of #99297

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants