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

support "pipe" function chaining syntax #2042

Merged
merged 3 commits into from Oct 2, 2017

Conversation

DanCech
Copy link
Member

@DanCech DanCech commented Oct 1, 2017

This PR adds support for chaining functions together by "piping" the output of each function into the next (as the first parameter), much like the way they're presented in the Grafana UI.

When dealing with deeply-nested queries this keeps the arguments for each function together, and can help increase readability vs the current "nested" syntax.

With this update the following query:

aliasByNode(movingAverage(sortByName(test.*),"5min"),1)

can be written as:

test.*|sortByName()|movingAverage("5min")|aliasByNode(1)

or:

sortByName(test.*)|movingAverage("5min")|aliasByNode(1)

Piping is supported within function arguments, so the same expression could also be written as

movingAverage(test.*|sortByName(),"5min")|aliasByNode(1)

A more useful example would be selecting series for the total parameter of asPercent:

test.*|sortByName()|asPercent(test.total|movingAverage("5min"))

The syntax is unwrapped in the parser and evaluator, so there is no difference in how the actual render functions are called, the evaluator walks through the piped calls from right to left, passing the chain up to that point into the function as the first parameter.

The one change I had to make that would affect existing syntax is that the pipe character | is added to the list of symbols that aren't allowed in pathExpressions, but I doubt that's widely used since it would make working with the whisper files painful. If a user does have series with a pipe in the name they can still be selected but the pipe would need to be escaped with a backslash.

@DanCech DanCech requested review from iksaif and deniszh Oct 1, 2017
@codecov-io
Copy link

@codecov-io codecov-io commented Oct 1, 2017

Codecov Report

Merging #2042 into master will increase coverage by 0.03%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2042      +/-   ##
==========================================
+ Coverage   69.69%   69.72%   +0.03%     
==========================================
  Files          78       78              
  Lines        7440     7448       +8     
  Branches     1440     1442       +2     
==========================================
+ Hits         5185     5193       +8     
  Misses       1969     1969              
  Partials      286      286
Impacted Files Coverage Δ
webapp/graphite/render/functions.py 87.58% <ø> (ø) ⬆️
webapp/graphite/render/evaluator.py 75% <100%> (+2.58%) ⬆️
webapp/graphite/render/grammar.py 83.33% <80%> (+0.64%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1769f81...350923f. Read the comment docs.

@DanCech
Copy link
Member Author

@DanCech DanCech commented Oct 1, 2017

Obviously this needs docs before it would be merge-able, I'd also like to write some tests for both the parser and evaluator since coverage is pretty weak there right now. Feedback would be appreciated before I go ahead with that though.

@iksaif
Copy link
Member

@iksaif iksaif commented Oct 1, 2017

That would certainly make it more readable.

@@ -14,7 +14,7 @@ def evaluateTarget(requestContext, target):
return result


def evaluateTokens(requestContext, tokens, replacements=None):
def evaluateTokens(requestContext, tokens, replacements=None, pipedArg=None):
Copy link
Member

@deniszh deniszh Oct 1, 2017

Choose a reason for hiding this comment

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

Could you please elaborate how pipedArg is passed here?

Copy link
Member Author

@DanCech DanCech Oct 1, 2017

Choose a reason for hiding this comment

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

pipedArg is passed when evaluating the rightmost piped function, the value passed in will be the parse tree for the remaining part of the expression. You can see on line 30 & 31 that the rightMost piped call is pop()ed off the stack and tokens is then passed as pipedArg when it's evaluated. This mechanism is how the piped calls are "converted" back to a regular nested set of function calls.

Copy link
Member

@deniszh deniszh Oct 1, 2017

Choose a reason for hiding this comment

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

Ah, missed that, sorry. Thanks!

Copy link
Member Author

@DanCech DanCech Oct 1, 2017

Choose a reason for hiding this comment

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

No worries, it's definitely the most non-obvious part of the change. I'll add some comments to explain what's going on there.

Copy link
Member Author

@DanCech DanCech Oct 2, 2017

Choose a reason for hiding this comment

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

Comments added

@deniszh
Copy link
Member

@deniszh deniszh commented Oct 1, 2017

But in general, I'm very 👍 👍 👍 about that change, very nice addition to function syntax.
Let's properly test and document this first, though.

deniszh
deniszh approved these changes Oct 1, 2017
iksaif
iksaif approved these changes Oct 2, 2017
@DanCech DanCech merged commit 8ea3ef0 into graphite-project:master Oct 2, 2017
2 checks passed
@DanCech DanCech deleted the pipe-syntax branch Oct 2, 2017
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.

None yet

4 participants