[GitHub] [lucene-solr] munendrasn commented on a change in pull request #1371: SOLR-14333: print readable version of CollapsedPostFilter query

[GitBox <gi...@apache.org>]

munendrasn commented on a change in pull request #1371:
URL: https://github.com/apache/lucene-solr/pull/1371#discussion_r491312573



##########
File path: solr/core/src/java/org/apache/solr/search/CollapsingQParserPlugin.java
##########
@@ -188,6 +206,11 @@ public int hashCode() {
       return 17 * (31 + selectorText.hashCode()) * (31 + type.hashCode());
     }
 
+    @Override
+    public String toString(){
+      return "groupHeadSelectorText=" +this.selectorText + ", groupHeadSelectorType=" +this.type;

Review comment:
       would it be better to include the class name too here?
   ```suggestion
         return "GroupHeadSelector[selectorText=" +this.selectorText + ", type=" +this.type + "]";
   ```

##########
File path: solr/core/src/java/org/apache/solr/search/CollapsingQParserPlugin.java
##########
@@ -128,6 +128,28 @@ field collapsing (with ngroups) when the number of distinct groups
   public static final String HINT_TOP_FC = "top_fc";
   public static final String HINT_MULTI_DOCVALUES = "multi_docvalues";
 
+  public enum NullPolicy {
+    IGNORE("ignore", 0),

Review comment:
       I agree with @madrob about removing the unused declarations but as access the public for these variables. I think we should deprecate it in this PR(for 8x) and remove it in different PR
   wdyt?

##########
File path: solr/core/src/java/org/apache/solr/search/CollapsingQParserPlugin.java
##########
@@ -279,7 +299,12 @@ public int getCost() {
     }
 
     public String toString(String s) {
-      return s;
+      return "{!collapse field=" + this.collapseField +
+          ", nullPolicy=" + this.nullPolicy.getName() + ", " +
+          this.groupHeadSelector.toString() +
+          (hint == null ? "": ", hint=" + this.hint) +
+          ", size=" + this.size
+          + "}";

Review comment:
       The above suggestion might lead cause this in debug response
   ```
   CollapsingPostFilter([CollapsingPostFilter field=id, nullPolicy=expand, GroupHeadSelector[selectorText=_version_ asc, type=SORT], hint=top_fc, size=5000])
   
   ```
   This is caused due to how Query is converted to String by `QueryParsing` Class but I think that should be handled separately as issue exists for different query type
    https://github.com/apache/lucene-solr/blob/ab3f1f0d1d0fcdbe58958197b11ba3837795a24a/solr/core/src/java/org/apache/solr/search/QueryParsing.java#L336

##########
File path: solr/core/src/java/org/apache/solr/search/CollapsingQParserPlugin.java
##########
@@ -279,7 +299,12 @@ public int getCost() {
     }
 
     public String toString(String s) {
-      return s;
+      return "{!collapse field=" + this.collapseField +
+          ", nullPolicy=" + this.nullPolicy.getName() + ", " +
+          this.groupHeadSelector.toString() +
+          (hint == null ? "": ", hint=" + this.hint) +
+          ", size=" + this.size
+          + "}";

Review comment:
       `{!collapse` specifies the query parser, I think we shouldn't include it in toString()
   ```suggestion
         return "CollapsingPostFilter[field=" + this.collapseField +
             ", nullPolicy=" + this.nullPolicy.getName() + ", " +
             this.groupHeadSelector +
             (hint == null ? "": ", hint=" + this.hint) +
             ", size=" + this.size
             + "]";
   ```




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org