Srushti gangireddy/advanced concept search fix #939

SrushtiGangireddy · 2018-06-15T15:58:38Z

Changed dialect in application.props to the MYSQLDialect
Changed the order of match in advancedconceptsearch
Changed the predicate a little
Added matchType and standardConcept to ConceptListResponse

danqrodney · 2018-06-15T18:09:05Z

common-api/src/main/java/org/pmiops/workbench/cdr/dao/ConceptDao.java


+    @Query(nativeQuery=true, value="select c.* from concept c join concept_relationship cr on c.concept_id=cr.concept_id_2 " +
+            "where cr.concept_id_1=?1 and cr.relationship_id='Maps to' ")
+    Concept findStandardConcept(long concept_id);


Is this definitely going to be limited to standard concept relationships? Is there guaranteed to only be one of these?

I got to check those.

danqrodney · 2018-06-15T18:21:20Z

common-api/src/main/java/org/pmiops/workbench/cdr/dao/ConceptService.java

                    // Optionally filter on standard concept, vocabulary ID, domain ID
                    if (standardConceptFilter.equals(StandardConceptFilter.STANDARD_CONCEPTS)) {
-                        predicates.add(criteriaBuilder.equal(root.get("standardConcept"),
+                        conceptName_Filter.add(criteriaBuilder.equal(root.get("standardConcept"),


I'm wondering if we need to change this logic to include "C" here (and exclude it below). WDYT?

Do you mean to include "C" concepts along with standard concepts and not return them as the part of non standard concepts ?

Yeah. I'm not sure it makes sense for now... if I understand correctly, the "C" concepts are standard but because we haven't calculated descendant counts, they all have zero counts right now. So this is probably fine for the time being... eventually we may want to return C concepts with descendant counts.

Added the check to include C concepts as part of standard concepts check and not include them as part of non standard concepts.

danqrodney · 2018-06-15T18:28:13Z

common-api/src/main/java/org/pmiops/workbench/cdr/dao/ConceptService.java

+

                    // Optionally filter on standard concept, vocabulary ID, domain ID
                    if (standardConceptFilter.equals(StandardConceptFilter.STANDARD_CONCEPTS)) {


I'm wondering if we should get rid of this code... given the logic where we return non-standard concepts that match code or ID unconditionally, it's not clear to me that filtering on standard vs. non-standard concepts really makes sense anymore. (It's weird to explicitly ask for standard concepts in the requests, and return non-standard ones anyway...)

So what would be the alternative ? We could make a filter that says "STANDARD_CONCEPTS_OR_CODE_MATCH" ?

No, just get rid of the filter entirely.

(We'd start out by keeping it in the API but ignoring it... and then remove it from the API.)

But we need to only get standard concepts . Are you saying just return standard concepts unless it is a code or concept_id match ?

Yep, that's what I'm suggesting. We could add some option back in later when we have requirements for fetching non-standard concepts on something else.

If you prefer we can keep this filter around for future use, but then we ought to have requests that ask for standard concepts get only standard concepts, non-standard get only non-standard and match on everything, etc.

Right now it is (MatchCode) or (MatchId) or (MatchName and standardconceptfilter). So may be it is fine like this for now. We can use the filter as how we want.

I was talking about adding a parameter to the SearchConceptRequest object . It would be an optional array with the match types you wanted. The array order sets the precedence. It would default to [Code, ConceptId , Name] .. But if you did want only standard concepts you would probably be searching on name. That option only makes sense searching on name. Unless a couple vocabularies use the same code and you would get more than one. It is rare in any case and there won't be enough matches to create noise imo.

We could also do : matchType: [ {type:Code, standard_concept:false},{type:ConceptId, standard_concept:false, {type:Name, standard_concept:true} ] -- so instead of array of scalars have array of objects and put the standard_concept filter in each match type object.

That said, its a lot of work for something we aren't using now. But given we can filter on domain, vocabulary, seems we should filter on standard_concept too just to be thorough. That said, given that the standard_concept filter only makes sense in a concept_name search, i think we are fine as we are. But Dan if you really don't like it , i'm fine taking it out for now until we need it.

I think the best option is to add another StandardConceptFilter.StandardOrCodeIdMatch ... Seems a good use of the StandardConceptFilter enumerator.

I'm OK with that, but then for the other options (standard, non-standard) we should return only those concepts... we shouldn't do ID matching for the other kind of concepts in those cases.

danqrodney · 2018-06-15T18:31:55Z

common-api/src/main/java/org/pmiops/workbench/cdr/dao/ConceptService.java

+
+
+                    Expression<Double> matchExp = criteriaBuilder.function("match", Double.class,
+                            root.get("conceptName"), criteriaBuilder.parameter(String.class , "keyword"));


Why the change to use a parameter instead of a literal?

danqrodney · 2018-06-15T18:32:18Z

common-api/src/main/java/org/pmiops/workbench/cdr/dao/ConceptService.java

                new Sort(Direction.DESC, "countValue"));
        NoCountFindAllDao<Concept, Long> conceptDao = new NoCountFindAllDao<>(Concept.class,
                entityManager);
+        conceptDao.keyword=keyword;


If we need to do this, let's pass it in to the constructor, and make the field final, instead of assigning it like this.

danqrodney · 2018-06-15T18:34:00Z

public-api/src/main/java/org/pmiops/workbench/publicapi/DataBrowserController.java

+
+                response.setMatchType("ConceptCode");
+
+                Concept std_concept = conceptDao.findStandardConcept(con.getConceptId());


If the matched concept is a standard concept, we don't need to do this lookup, do we? It looks like you have that check below in the next if block...

danqrodney · 2018-06-15T18:34:17Z

public-api/src/main/java/org/pmiops/workbench/publicapi/DataBrowserController.java

-        response.setItems(concepts.getContent().stream().map(TO_CLIENT_CONCEPT).collect(Collectors.toList()));
+        List<Concept> matchedConcepts = concepts.getContent();
+        List<Concept> conceptCodeMatches = new ArrayList<>();
+        for(Concept con:matchedConcepts){


Add a space after the colon

danqrodney · 2018-06-15T18:34:53Z

public-api/src/main/java/org/pmiops/workbench/publicapi/DataBrowserController.java

+        for(Concept con:matchedConcepts){
+            String conceptCode = con.getConceptCode();
+            Long conceptId = con.getConceptId();
+            if(conceptCode.equals(searchConceptsRequest.getQuery())) {


It seems like we should be looking for a non-standard code in the results, rather than doing this if and the next if statement...

Seems nicer. Thanks

danqrodney · 2018-06-15T18:37:31Z

public-api/src/main/java/org/pmiops/workbench/publicapi/DataBrowserController.java

+                Concept std_concept = conceptDao.findStandardConcept(con.getConceptId());
+                response.setStandardConcept(TO_CLIENT_CONCEPT.apply(std_concept));
+
+                response.setItems(conceptCodeMatches.stream().map(TO_CLIENT_CONCEPT).collect(Collectors.toList()));


So it's possible for us to have a query string that both matches a concept code or ID and also appears as a keyword in standard concepts. If that happens, what do we want the UX to be?

If we want to return the other standard concepts in these cases (which seems right to me), we shouldn't return a response here... we should just tack on a non-standard concept with the standard concept info filled out and continue. You could even do this logic as a part of TO_CLIENT_CONCEPT and leave the logic in here the way it was before.

I thought if there is a match on concept code/id we are going to return the concept matched by code/id and throw away the rest of concept_name matches

Yeah, I'm not sure whether this is the right behavior. It may be that some codes / concepts are short and show up in other things that researchers are looking for... might be better to always do the full search, even when there's a code / ID match. @biopete WDYT?

danqrodney · 2018-06-18T14:20:48Z

public-api/src/main/resources/public-api.yaml

        matchType:
          type: string
          description: match column type
        standardConcept:


You might want to rename this "standardConcepts".

Renamed the field to standardConcepts.

danqrodney · 2018-06-18T14:21:06Z

common-api/src/main/java/org/pmiops/workbench/cdr/dao/ConceptDao.java

    @Query(nativeQuery=true, value="select c.* from concept c join concept_relationship cr on c.concept_id=cr.concept_id_2 " +
            "where cr.concept_id_1=?1 and cr.relationship_id='Maps to' and c.standard_concept='S' ")
-    Concept findStandardConcept(long concept_id);
+    List<Concept> findStandardConcept(long concept_id);


Rename to findStandardConcepts.

danqrodney · 2018-06-18T14:24:53Z

public-api/src/main/java/org/pmiops/workbench/publicapi/DataBrowserController.java

+            String conceptCode = con.getConceptCode();
+            Long conceptId = con.getConceptId();
+            if(!con.getStandardConcept().equals("S")){
+                if(conceptCode.equals(searchConceptsRequest.getQuery())) {


This code and the next code are identical apart from the "matchType" right? I think you can simplify this code and just have:

response.setMatchType(conceptCode.equals(searchConceptsRequest.getQuery()) ? "ConceptCode" : "ConceptId")

and get rid of the if/else.

Makes sense.

danqrodney · 2018-06-18T14:25:20Z

public-api/src/main/resources/public-api.yaml

            $ref: "#/definitions/Concept"
-
+        matchType:
+          type: string


Consider making this an enum instead of a String.

danqrodney · 2018-06-18T14:26:05Z

public-api/src/main/java/org/pmiops/workbench/publicapi/DataBrowserController.java

+
+                    response.setItems(conceptCodeMatches.stream().map(TO_CLIENT_CONCEPT).collect(Collectors.toList()));
+
+                    return ResponseEntity.ok(response);


(As mentioned previously, I wouldn't necessarily return just the non-standard concept here, but I guess this is a product question.)

Changed the method to return the non standard ones along with the standard ones they are mapped to and any name concept matches too.

danqrodney · 2018-06-18T14:28:48Z

api/src/test/java/org/pmiops/workbench/api/ConceptsControllerTest.java

+    saveConcepts();
+    assertResults(
+            conceptsController.searchConcepts("ns", "name",
+                    new SearchConceptsRequest().query("conceptD")), CLIENT_CONCEPT_4 , CLIENT_CONCEPT_6);


I'm confused about how this currently passes... CLIENT_CONCEPT_6 is non-standard. Doesn't your code below only return a single concept in this case?

Either way, please add a test that asserts that standard concepts are returned with the non-standard one.

I will add one.
The search matches on the code and fetches CLIENT_CONCEPT_4 and matches on the name an d fetches CLIENT_CONCEPT_5 and 6 irrespective of the standard concept filter because the query does not specify the filter.

biopete · 2018-06-21T17:54:44Z

common-api/src/main/java/org/pmiops/workbench/cdr/dao/ConceptService.java

+

                    // Optionally filter on standard concept, vocabulary ID, domain ID
                    if (standardConceptFilter.equals(StandardConceptFilter.STANDARD_CONCEPTS)) {


So what would be the alternative ? We could make a filter that says "STANDARD_CONCEPTS_OR_CODE_MATCH" ?

biopete · 2018-06-21T17:56:51Z

public-api/src/main/java/org/pmiops/workbench/publicapi/DataBrowserController.java

    }

    @Override
    public ResponseEntity<ConceptListResponse> getAdvancedConceptSearch(SearchConceptsRequest searchConceptsRequest){


Per convo with Srushti where we are getting rid of the non advanced concept search, I think we should name this the same as it is in the other api -- searchConcepts or whatever ? So the public api and other apis have same name.

Got rid of the old search concepts method and changed the name of advanced concept search to searchConcepts similar to the one in api.

biopete · 2018-06-21T17:59:58Z

common-api/src/main/java/org/pmiops/workbench/cdr/dao/ConceptDao.java

    nativeQuery = true)
    List<Concept> findAllConceptsOrderedByCount(@Param("standard_concept") String standard_concept);

+    @Query(value = "select c.* from concept c order by c.count_value desc", nativeQuery = true)


This is just the same as findAllConceptsOrderedByCount() but no parameter for stardard concept.
I think we need to have findAllConceptsOrderedByCount(@param standard , @optional param maxresults) and use a limit max results. ..

We have to have the standard concepts option ... righ now we are getting everything which is no good.

Added the optional max results parameter to the old method. Can fetch everything by passing in the max value in case the param is not present.

biopete · 2018-06-21T18:06:10Z

public-api/src/main/java/org/pmiops/workbench/publicapi/DataBrowserController.java

+        if(searchConceptsRequest.getQuery() == null || searchConceptsRequest.getQuery().isEmpty()){
+            List<Concept> concepts = conceptDao.findConceptsOrderedByCount();
+            ConceptListResponse response = new ConceptListResponse();
+            if(maxResults < concepts.size()){


I'm no java expert -- but this code looks to me like the limit should be put on the mysql query in findAllConceptsOrderedByCount.. See above comment on the conceptDao. Not sure if Java would return all 4million concepts and then filter that list. Probably not.

Yeah, this should be a parameter to findConceptsOrderedByCount.

biopete · 2018-06-21T18:11:27Z

public-api/src/main/resources/public-api.yaml

          schema:
            $ref: "#/definitions/DbDomainListResponse"

  /v1/databrowser/advanced-concept-search:


Let's make sure this is the same api name/path as the private api

So the workbench API actually has the workspace ID in the path (since we will eventually be using workspace metadata when querying for stuff). We could name it similarly, but the paths are going to differ.

cool. I like things named the same that are the same.

Changed the name to searchConcepts now that the old concept-search is removed.

… take in limit, changed the name of api to searchConcepts

biopete · 2018-06-21T20:28:29Z

api/db-cdr/generate-cdr/make-bq-data.sh

+set c.count_value = r.count_value,c.source_count_value=r.source_count_value
+from  (select cast(r.stratum_1 as int64) as concept_id, count_value , source_count_value
+from \`$WORKBENCH_PROJECT.$WORKBENCH_DATASET.achilles_results\` r
+where r.analysis_id in (2,4,5) and CAST(r.stratum_1 as int64) > 0 group by r.stratum_1, r.count_value, r.source_count_value) as r


I don't think we need a separate query for this, just add the in clause to the above query like : r.analysis_id in (3000,2,4,5) ...

Changed the query.

biopete · 2018-06-21T22:18:11Z

public-api/src/main/java/org/pmiops/workbench/publicapi/DataBrowserController.java

        ConceptListResponse response = new ConceptListResponse();
-        response.setItems(concepts.getContent().stream().map(TO_CLIENT_CONCEPT).collect(Collectors.toList()));
+        List<Concept> matchedConcepts = concepts.getContent();
+        for(Concept con : matchedConcepts){


I don't think this logic is right. Fore every result that is not a standard concept , we want to get the standardConcepts and set it on the resp.standardConcepts?
Don't we just want to do this if we matched on concept_code or concept_id and we only have one result ?

Messed up a line. Added back.

biopete · 2018-06-22T00:39:36Z

I'm out tomorrow. I've tested this and functionally it works. I'm ok if api changes a little and the standard concept filter is removed or whatever. So i will approve pending Dan's approval in hopes of this pr getting merged in. Then I can add my public ui in.

biopete

Pending Dan's approval. I'm out tomorrow. Thanks!

danqrodney · 2018-06-22T15:36:28Z

common-api/src/main/java/org/pmiops/workbench/cdr/dao/ConceptService.java

                    // Optionally filter on standard concept, vocabulary ID, domain ID
                    if (standardConceptFilter.equals(StandardConceptFilter.STANDARD_CONCEPTS)) {
+
+                        predicates.add(


It might be simpler to make a conceptNamePredicate used here and in the next if block; and only make conceptName_filter in the else case.

danqrodney · 2018-06-22T15:38:22Z

common-api/src/main/java/org/pmiops/workbench/cdr/dao/ConceptService.java

+                        );
+
+
+                    } else if (standardConceptFilter.equals(StandardConceptFilter.STANDARD_OR_CODE_ID_MATCH)) {


So we may want to make this filter be the default behavior if no filter is specified in the request. If we don't, you should have an else block after this that adds the logic that used to be on lines 125-129... we should always apply a filter on name / ID. (And you should make sure DataBrowserController passes this filter in.)

danqrodney · 2018-06-22T16:26:44Z

public-api/src/main/java/org/pmiops/workbench/publicapi/DataBrowserController.java

+            String conceptCode = con.getConceptCode();
+            String conceptId = String.valueOf(con.getConceptId());
+
+                if(!con.getStandardConcept().equals("S") && (searchConceptsRequest.getQuery().equals(conceptCode) || searchConceptsRequest.getQuery().equals(conceptId))){


Fix indentation please.

SrushtiGangireddy added 8 commits June 14, 2018 11:08

Advanced concept search dialect fix

810854b

search codes

b8165df

added match type and standard concept

e1a7505

added match type and standard concept

5c85085

Working predicate change

d57596b

Changed predicate

b3a978a

Codacy fix

6f54c58

Codacy fix

de53aa8

SrushtiGangireddy requested review from biopete and danqrodney June 15, 2018 16:57

Tests to test the order of match

53719c6

danqrodney reviewed Jun 15, 2018

View reviewed changes

SrushtiGangireddy added 4 commits June 15, 2018 15:04

standard concept fetch fix

e1794a2

standard concept fetch fix

d28e1b1

standard concept fetch fix

a4c5084

multiple standard concept fix

6d1ef38

danqrodney reviewed Jun 18, 2018

View reviewed changes

SrushtiGangireddy added 11 commits June 19, 2018 10:45

changed match type to enum

c0c5154

added c to standard concept check

ed756f7

Test added to fetch concepts from concept relationship

14cef89

concept relationship test

ab1b99b

Codacy fix

441c2b6

Controller Test

c2cf5d4

Data Browser Controller Test

8097210

Removing unused imports

4f80067

data browser controller tests

5d2173c

empty search

afbf889

test

4d1c3d7

biopete reviewed Jun 21, 2018

View reviewed changes

Got rid of conceptsSearch and changed the findAllConceptsOrderedBy to…

8c95534

… take in limit, changed the name of api to searchConcepts

SrushtiGangireddy added 2 commits June 21, 2018 15:02

commented ref to getConceptsSearch

1dc4188

Codacy fix

bbe8c1e

biopete reviewed Jun 21, 2018

View reviewed changes

SrushtiGangireddy added 2 commits June 21, 2018 17:42

query update

99c637c

query update

302a8be

biopete approved these changes Jun 22, 2018

View reviewed changes

Added standardcode or id match enum

0d767d8

danqrodney reviewed Jun 22, 2018

View reviewed changes

tweaks to the predicate addition

050bf9d

danqrodney approved these changes Jun 22, 2018

View reviewed changes

indentation tweaks

e61573c

SrushtiGangireddy merged commit 899e280 into master Jun 22, 2018

SrushtiGangireddy deleted the SrushtiGangireddy/advancedConceptSearchFix branch June 22, 2018 17:02



		// Optionally filter on standard concept, vocabulary ID, domain ID
		if (standardConceptFilter.equals(StandardConceptFilter.STANDARD_CONCEPTS)) {



		Expression<Double> matchExp = criteriaBuilder.function("match", Double.class,
		root.get("conceptName"), criteriaBuilder.parameter(String.class , "keyword"));


		response.setMatchType("ConceptCode");

		Concept std_concept = conceptDao.findStandardConcept(con.getConceptId());


		response.setItems(conceptCodeMatches.stream().map(TO_CLIENT_CONCEPT).collect(Collectors.toList()));

		return ResponseEntity.ok(response);

		);


		} else if (standardConceptFilter.equals(StandardConceptFilter.STANDARD_OR_CODE_ID_MATCH)) {

Srushti gangireddy/advanced concept search fix #939

Srushti gangireddy/advanced concept search fix #939

Uh oh!

Conversation

SrushtiGangireddy commented Jun 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!