[GitHub] [lucene-solr] dsmiley commented on a change in pull request #2356: SOLR-15152: Export Tool should export nested docs cleanly in .json, .jsonl, and javabin

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

dsmiley commented on a change in pull request #2356:
URL: https://github.com/apache/lucene-solr/pull/2356#discussion_r575699438



##########
File path: solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleTests.java
##########
@@ -1935,21 +1935,22 @@ public void testChildDoctransformer() throws IOException, SolrServerException {
     Map<String,SolrInputDocument> allDocs = new HashMap<>();
 
     for (int i =0; i < numRootDocs; i++) {
-      client.add(genNestedDocuments(allDocs, 0, maxDepth));
+      client.add(generateNestedDocuments(allDocs, 0, maxDepth));
     }
 
     client.commit();
 
     // sanity check
-    SolrQuery q = new SolrQuery("q", "*:*", "indent", "true");
+    SolrQuery q = new SolrQuery("q", "*:*", "rows", "0");
     QueryResponse resp = client.query(q);
     assertEquals("Doc count does not match",
         allDocs.size(), resp.getResults().getNumFound());
 
 
-    // base check - we know there is an exact number of these root docs
-    q = new SolrQuery("q","level_i:0", "indent", "true");
-    q.setFields("*", "[child parentFilter=\"level_i:0\"]");
+    // base check - we know there the exact number of these root docs
+    q = new SolrQuery("{!parent which=\"*:* -_nest_path_:*\"}");
+    q.addField("*,[child limit=\"-1\"]");

Review comment:
       I think it was clearer before with setFields (plural) as you are adding two fields

##########
File path: solr/core/src/java/org/apache/solr/response/transform/ChildDocTransformerFactory.java
##########
@@ -160,8 +161,18 @@ protected static String processPathHierarchyQueryString(String queryString) {
     int indexOfLastPathSepChar = queryString.lastIndexOf(PATH_SEP_CHAR, indexOfFirstColon);
     if (indexOfLastPathSepChar < 0) {
       // regular filter, not hierarchy based.
-      return ClientUtils.escapeQueryChars(queryString.substring(0, indexOfFirstColon))

Review comment:
       I think there should **not** have been escaping here in the first place.  Unless the user is using the special syntax, we should handle it plainly like we do everywhere else -- simple.  It may mean the user has to escape characters like '/' but that's normal.  In a Java String that has an embedded local-params for the child doc transformer, that winds up being four back-slashes.  So be it.  It may seem like we're trying to do the user a favor by escaping what we see but I think it's actually more confusing to reason about because it's inconsistent.  Besides, there are other ways of constructing the query syntax that results in zero escaping.  like `childFilter='{!field f=_nest_path_}/toppings'`  So that's longer but no escaping at least.  Up to the user's prerogative.
   CC @moshebla
   
   I'll fix this in your PR momentarily.




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