-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Ensure stability of clause order for DisjunctionMaxQuery toString #13944
Ensure stability of clause order for DisjunctionMaxQuery toString #13944
Conversation
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.
I'm a bit unhappy about making toString()
run in O(n log(n)) with the number of clauses. Could we instead save a list copy of the queries in the constructor and use the list for toString()
? BooleanQuery
does something similar.
Thanks for the feedback. Looking at |
See |
I needed to sort the
Not sure if it's the right way. |
I wouldn't sort them, and just rely on the order that the caller supplied? |
Ha, I see. Could we say that the new |
Yes, exactly. |
Can you add an entry to |
Done. Thanks for reviewing! |
…3944) Co-authored-by: Laurent <jakubinal@lexum.com>
Thanks for this! I had to redo a bunch of tests over this matter at work. |
Since #110, the disjuncts elements of DisjunctionMaxQueries don't have an order anymore, which is impacting the
toString
method. In isolation, that does not matter. But, in Solr, when the debug component is needed for a distributed query, every shard can return a different toString representation of the same query... and the different toString keys of the debug response will have an array value, containing those different representations (instead of having one value for one same representation).Example with the
parsedquery_toString
key (of a json response within Solr):parsedquery_toString":["((docIdentifiers:\"Okarandeep Osingh\" docIdentifiers:Otest) | (docTitle:\"Okarandeep Osingh\" docTitle:Otest) | (docBody:\"Okarandeep Osingh\" docBody:Otest))","((docBody:\"Okarandeep Osingh\" docBody:Otest) | (docTitle:\"Okarandeep Osingh\" docTitle:Otest) | (docIdentifiers:\"Okarandeep Osingh\" docIdentifiers:Otest))"]
When PR110 was merged, Solr adapted its unit tests this way: apache/solr#117 but, later on within Lucene, the toString method of DisjuctionIntervalsSource was adapted in prevision of a potential similar future change: #193.
I adapted the toString method of DisjunctionMaxQueries similarly to this PR.