-
Notifications
You must be signed in to change notification settings - Fork 852
SOLR-18328 Add support for standard deviation in rollup for streaming expressions #4691
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| title: "Support 'std' (standard deviation) metric in rollup for streaming expressions" | ||
| type: added | ||
| authors: | ||
| - name: khushjain | ||
| links: | ||
| - name: SOLR-18328 | ||
| url: https://issues.apache.org/jira/browse/SOLR-18328 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,16 +23,15 @@ | |
| import org.apache.solr.client.solrj.io.stream.expr.StreamExpressionParameter; | ||
| import org.apache.solr.client.solrj.io.stream.expr.StreamFactory; | ||
|
|
||
| /** | ||
| * Metric that computes the sample standard deviation of a numeric column over a stream. Consistent | ||
| * with the {@code std} streaming evaluator. | ||
| */ | ||
| public class StdMetric extends Metric { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you include java docs for this class ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added docstrings. |
||
| // How'd the MeanMetric get to be so mean? | ||
| // Maybe it was born with it. | ||
| // Maybe it was mayba-mean. | ||
| // | ||
| // I'll see myself out. | ||
|
|
||
| private String columnName; | ||
| private double doubleSum; | ||
| private long longSum; | ||
| private double sum; | ||
| private double sumSq; | ||
| private long count; | ||
|
|
||
| public StdMetric(String columnName) { | ||
|
|
@@ -75,21 +74,45 @@ private void init(String functionName, String columnName, boolean outputLong) { | |
| } | ||
|
|
||
| @Override | ||
| public void update(Tuple tuple) {} | ||
| public void update(Tuple tuple) { | ||
| Object o = tuple.get(columnName); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could the object 'o' be made immutable by declaring it as final ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it could be added but the current state is consistent with the existing style. |
||
| double val; | ||
| if (o instanceof Double d) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could the 'if-else' structure be simplified ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there definitly are some unusla coding patterns in the streaming code... but we tend to follow them once they exist as there are so many of them!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @epugh Thanks for the clarification :) Is there a documentation about the code standard adopted by the codebase ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we have "tidy" and errrorprone tools that we run int he builds. As far as in the streaming code, nothign formal written, just, look at other java classes ;-). Having said that, I do appreciate your reviewing this PR, and there are lots of PR's out there that need a reviewer to go through them!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed on the quirky instanceof pattern, but keeping it consistent with all its siblings.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @epugh I would be happy to review the PR's :) |
||
| val = d; | ||
| } else if (o instanceof Float f) { | ||
| val = f.doubleValue(); | ||
| } else if (o instanceof Integer i) { | ||
| val = i.doubleValue(); | ||
| } else if (o instanceof Long l) { | ||
| val = l.doubleValue(); | ||
| } else { | ||
| return; | ||
| } | ||
| ++count; | ||
| sum += val; | ||
| sumSq += val * val; | ||
| } | ||
|
|
||
| @Override | ||
| public Metric newInstance() { | ||
| return new MeanMetric(columnName, outputLong); | ||
| return new StdMetric(columnName, outputLong); | ||
| } | ||
|
|
||
| @Override | ||
| public String[] getColumns() { | ||
| return new String[] {columnName}; | ||
| } | ||
|
|
||
| /** Returns the sample standard deviation of the values seen so far. */ | ||
| @Override | ||
| public Number getValue() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you include javadocs to understand the purpose of this method ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added docstrings. |
||
| return null; | ||
| double std = | ||
| count <= 1 ? 0.0d : Math.sqrt(((count * sumSq) - (sum * sum)) / (count * (count - 1.0D))); | ||
| if (outputLong) { | ||
| return Math.round(std); | ||
| } else { | ||
| return std; | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any more docs on these indivdual ones or is this mention what the pattern is? Just wondering if folks will know how to use it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no dedicated per-metric docs exists, all streaming metrics are documented this same inline way.
Support for
missing(col),countDist(col)andper(col, percentile)are also added by me in the past.