Skip to content
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

Lucene Query Builder Unification #2802

Merged
merged 14 commits into from Jul 16, 2014
Merged

Conversation

joshmoore
Copy link
Member

With the migration of @dominikl's byLuceneQueryBuilder() method to the server in gh-2676 all other clients (OMERO.web and the CLI) need to be updated to make use of the single implementation. Testing involves showing that results from any one of the clients match the other 2 clients. Search options which should be involved include:

  • Various search types (Project, Dataset, ...)
  • Various fields ("name", "description", "annotation")
  • "acqusition date" v. "import date"

Testing user/group semantics via the CLI might be considered out-of-scope.

Follow-ons to this PR will include:

  • @joshmoore: modify server-side implementation to better handle some queries added here with aa21d07
  • @will-moore & @dominikl : modify what values are passed by the clients and how results are displayed.

Note: This PR replaces gh-2784

joshmoore and others added 12 commits July 15, 2014 12:03
Rather than have 2 implementations of the query parsing
logic, omero.gateway now uses the byLuceneQueryBuilder
method to have the custom query built server-side.
Now the date query of byLuceneQueryBuilder can be based
on either the import date or the acquisitionDate by setting
the new parameter.
SearchControl was not properly making use of the HqlControl
_configure method. With this commit, all of the options from
HqlControl are also provided for regular search, including
output style and `--all` for querying across groups.
Some searches like '%' generated empty strings in LQB
which led to invalid query exceptions "<EOF>" etc.
@snoopycrimecop
Copy link
Member

Conflicting PR. Removed from build OME-5.0-merge-push#110. See the console output for more details.

@will-moore
Copy link
Member

This all seems to be behaving as expected.
Getting identical results in both clients.
Some comments / suggestions / questions:

  • Web doesn't allow you to pick a user from within 'All Groups' but Insight does. Probably should update web
  • We currently search by Acquisition Date by default, but this is ignored for non-images. Maybe the form should have Import Date as the default?
  • Insight could maybe show a "No results" placeholder instead of blank center panel (leaves you unsure whether search has finished).
  • Might be nice if Insight could make the search results rows a bit thinner, just so that more are visible without scrolling the page.

None of these are blockers (or even related to this PR?) so: good to merge.

@joshmoore
Copy link
Member Author

Thanks, @will-moore.

joshmoore added a commit that referenced this pull request Jul 16, 2014
@joshmoore joshmoore merged commit cccb1a5 into ome:dev_5_0 Jul 16, 2014
@joshmoore joshmoore deleted the lucenequerybuilder branch July 16, 2014 15:22
@dominikl
Copy link
Member

@will-moore Sounds reasonable; I'm going to make your proposed changes for Insight in another PR.

@joshmoore
Copy link
Member Author

--rebased-to #2858

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants