Re: [PR] SOLR-18220 Add support for countDist in rollup for streaming expressions [solr]

2026-05-08 Thread via GitHub 


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]

2026-05-08 Thread via GitHub 


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]

2026-05-04 Thread via GitHub 


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]

2026-05-04 Thread via GitHub 


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]

2026-05-04 Thread via GitHub 


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]

2026-05-04 Thread via GitHub 


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]

2026-05-04 Thread via GitHub 


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]

2026-05-04 Thread via GitHub 


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]

2026-05-04 Thread via GitHub 


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]

2026-05-04 Thread via GitHub 


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]

[PR] SOLR-18220 Add support for countDist in rollup for streaming expressions [solr]

2026-05-03 Thread via GitHub 


KhushJain opened a new pull request, #4394:
URL: https://github.com/apache/solr/pull/4394

   https://issues.apache.org/jira/browse/SOLR-X
   
   
   
   
   # Description



   
   `Rollup` function to support `countDist` (count distinct) statistics in 
`/stream` handler.  

 
   # Solution   

 
   Implement the existing `CountDistinctMetric` stub in the streaming 
expressions framework:  
   

   **`CountDistinctMetric.java`**: 
   1. Was a no-op stub. `update()` did nothing and `getValue()` returned null. 
Now uses `HashSet` to track distinct non-null values per group. 
   2. Fixed `toExpression()` which emitted a spurious `outputLong` parameter 
producing malformed expressions like `countDist(a_i,true)`
   3. Added parameter count validation in the`StreamExpression` constructor.





 
   # Tests  

 
   Updated existing tests in `StreamDecoratorTest.java` and 
`StreamingTest.java`:   
 

   - **`StreamDecoratorTest.testRollupStream`**: Added `countDist(a_i)` and 
`countDist(a_s)` to the expression and asserted.
   - **`StreamDecoratorTest.testHashRollupStream`**: Same additions for 
hash-based rollup.
   - **`StreamingTest.testRollupStream`**: Added `CountDistinctMetric("a_i")` 
and `CountDistinctMetric("a_s")` to the metrics array and asserted, including 
the null grouping field test.
   
   # Checklist
   
   Please review the following and check all that apply:
   
   - [x] I have reviewed the guidelines for [How to 
Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) and my 
code conforms to the standards described there to the best of my ability.
   - [x] I have created a Jira issue and added the issue ID to my pull request 
title.
   - [x] I have given Solr maintainers 
[access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork)
 to contribute to my PR branch. (optional but recommended, not available for 
branches on forks living under an organisation)
   - [x] I have developed this patch against the `main` branch.
   - [x] I have run `./gradlew check`.
   - [x] I have added tests for my changes.
   - [x] I have added documentation for the [Reference 
Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
   - [x] I have added a [changelog 
entry](https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc) for my 
change
   


-- 
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]