Re: [PR] SOLR-18220 Add support for countDist in rollup for streaming expressions [solr]
KhushJain commented on PR #4394: URL: https://github.com/apache/solr/pull/4394#issuecomment-4410102874 Thanks @epugh! Can we backport this to 9.x as well? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-18220 Add support for countDist in rollup for streaming expressions [solr]
epugh merged PR #4394: URL: https://github.com/apache/solr/pull/4394 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-18220 Add support for countDist in rollup for streaming expressions [solr]
KhushJain commented on PR #4394: URL: https://github.com/apache/solr/pull/4394#issuecomment-4372123421 @epugh The PR check failed on 2 existing flaky tests on CloudConsistency. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-18220 Add support for countDist in rollup for streaming expressions [solr]
KhushJain commented on code in PR #4394:
URL: https://github.com/apache/solr/pull/4394#discussion_r3182460312
##
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/metrics/CountDistinctMetric.java:
##
@@ -81,14 +90,11 @@ public String[] getColumns() {
@Override
public Number getValue() {
-// No op for now
-return null;
+return distinctValues.size();
}
@Override
public StreamExpressionParameter toExpression(StreamFactory factory) throws
IOException {
-return new StreamExpression(getFunctionName())
-.withParameter(columnName)
-.withParameter(Boolean.toString(outputLong));
+return new StreamExpression(getFunctionName()).withParameter(columnName);
Review Comment:
Added test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-18220 Add support for countDist in rollup for streaming expressions [solr]
KhushJain commented on code in PR #4394:
URL: https://github.com/apache/solr/pull/4394#discussion_r3182399772
##
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/metrics/CountDistinctMetric.java:
##
@@ -81,14 +90,11 @@ public String[] getColumns() {
@Override
public Number getValue() {
-// No op for now
-return null;
+return distinctValues.size();
}
@Override
public StreamExpressionParameter toExpression(StreamFactory factory) throws
IOException {
-return new StreamExpression(getFunctionName())
-.withParameter(columnName)
-.withParameter(Boolean.toString(outputLong));
+return new StreamExpression(getFunctionName()).withParameter(columnName);
Review Comment:
This is the SQL module's map-reduce path, not the streaming expression path.
This PR targets countDist in streaming expression rollup()
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-18220 Add support for countDist in rollup for streaming expressions [solr]
KhushJain commented on code in PR #4394:
URL: https://github.com/apache/solr/pull/4394#discussion_r3182354667
##
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/metrics/CountDistinctMetric.java:
##
@@ -53,6 +55,10 @@ public CountDistinctMetric(StreamExpression expression,
StreamFactory factory)
expression,
functionName));
}
+if (1 != expression.getParameters().size()) {
+ throw new IOException(
+ String.format(Locale.ROOT, "Invalid expression %s - unknown operands
found", expression));
Review Comment:
The old constructor silently ignored the second parameter anyway, so no one
could have depended on it. This PR fixes the serialization to match what the
constructor actually accepts
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-18220 Add support for countDist in rollup for streaming expressions [solr]
KhushJain commented on code in PR #4394:
URL: https://github.com/apache/solr/pull/4394#discussion_r3182320671
##
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/metrics/CountDistinctMetric.java:
##
@@ -66,7 +72,10 @@ private void init(String functionName, String columnName) {
@Override
public void update(Tuple tuple) {
-// Nop for now
+Object value = tuple.get(columnName);
+if (value != null) {
+ distinctValues.add(value);
+}
Review Comment:
`APPROX_COUNT_DISTINCT / hll` constant existed but the code was a complete
no-op. I'd argue that's a separate feature and needs its own function
registered like `hll()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-18220 Add support for countDist in rollup for streaming expressions [solr]
KhushJain commented on PR #4394: URL: https://github.com/apache/solr/pull/4394#issuecomment-4371920187 > So was this a partly started feature that was never finished?? The class existed and worked for facets and stats where it was just used as an identifier that gets push down to json facet api to do the actual computation. So update()/getValue() was never called in that workflow. For rollup, it needs implementations for these methods to work. Currently countDist was just returning null. So even though the method is registered, it would have just returned null when invoked. This PR fills in that implementation. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-18220 Add support for countDist in rollup for streaming expressions [solr]
Copilot commented on code in PR #4394:
URL: https://github.com/apache/solr/pull/4394#discussion_r3181449818
##
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/metrics/CountDistinctMetric.java:
##
@@ -66,7 +72,10 @@ private void init(String functionName, String columnName) {
@Override
public void update(Tuple tuple) {
-// Nop for now
+Object value = tuple.get(columnName);
+if (value != null) {
+ distinctValues.add(value);
+}
Review Comment:
`CountDistinctMetric(String, true)` is already used for SQL
`APPROX_COUNT_DISTINCT`, but this implementation ignores that mode and still
retains every distinct value in a `HashSet`. That means the "approximate"
variant now has exact-count memory growth, which can turn grouped queries over
high-cardinality fields into large heap usage instead of the bounded-memory HLL
behavior callers expect.
##
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/metrics/CountDistinctMetric.java:
##
@@ -81,14 +90,11 @@ public String[] getColumns() {
@Override
public Number getValue() {
-// No op for now
-return null;
+return distinctValues.size();
}
@Override
public StreamExpressionParameter toExpression(StreamFactory factory) throws
IOException {
-return new StreamExpression(getFunctionName())
-.withParameter(columnName)
-.withParameter(Boolean.toString(outputLong));
+return new StreamExpression(getFunctionName()).withParameter(columnName);
Review Comment:
This new expression form is still incomplete in the distributed
SQL/map-reduce path: `SolrTable.handleGroupByMapReduce()` builds the worker
`StreamFactory` with `sum/min/max/avg/count` only, so a `ParallelStream` that
pushes `rollup(..., countDist(...))` to workers will still fail when
`numWorkers > 1`. The feature won't actually be usable end-to-end for parallel
group-by queries until that factory also registers `countDist`.
##
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/metrics/CountDistinctMetric.java:
##
@@ -53,6 +55,10 @@ public CountDistinctMetric(StreamExpression expression,
StreamFactory factory)
expression,
functionName));
}
+if (1 != expression.getParameters().size()) {
+ throw new IOException(
+ String.format(Locale.ROOT, "Invalid expression %s - unknown operands
found", expression));
Review Comment:
Previous releases serialized this metric as `countDist(field,true)` from
`toExpression()`. Rejecting any 2-argument form here makes those
already-generated expressions fail to parse after upgrade instead of remaining
backward compatible while you fix the serializer.
##
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/metrics/CountDistinctMetric.java:
##
@@ -81,14 +90,11 @@ public String[] getColumns() {
@Override
public Number getValue() {
-// No op for now
-return null;
+return distinctValues.size();
}
@Override
public StreamExpressionParameter toExpression(StreamFactory factory) throws
IOException {
-return new StreamExpression(getFunctionName())
-.withParameter(columnName)
-.withParameter(Boolean.toString(outputLong));
+return new StreamExpression(getFunctionName()).withParameter(columnName);
Review Comment:
The PR fixes `CountDistinctMetric.toExpression()` and tightens its parser,
but there is still no dedicated round-trip regression test for `countDist`
alongside the existing metric `toExpression()` tests in
`StreamExpressionToExpressionTest`. Without that, the exact serialization bug
fixed here is easy to reintroduce unnoticed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-18220 Add support for countDist in rollup for streaming expressions [solr]
epugh commented on PR #4394: URL: https://github.com/apache/solr/pull/4394#issuecomment-4370913398 So was this a partly started feature that was never finished?? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
