Trunk-5680 - For Debug reasons - Refactor getSimilarPeople to use Lucene #3116

fruether · 2020-01-13T20:03:28Z

Description of what I changed

This PR is mainly to discuss the subject in the following thread: https://talk.openmrs.org/t/soundex-search-in-lucenequeries/26068/10

So please stop focusing on beautification/formal issues of this PR: It is not intended to be merged into the code base. Focus right now is to make the code work. After!!!!!!!!!, it is working I will of cause re-work on the code and make it match the formal and quality criteria. So please focus your review effort on making it work or spend the time on another PR. Thank you!

Issue I worked on

see https://issues.openmrs.org/browse/Trunk-5680

Checklist: I completed these to help reviewers :)

My pull request only contains ONE single commit
(the number above, next to the 'Commits' tab is 1).

No? -> read here on how to squash multiple commits into one
My IDE is configured to follow the code style of this project.

No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend
I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)

No? -> write tests and add them to this commit git add . && git commit --amend
I ran mvn clean package right before creating this pull request and
added all formatting changes to my commit.

No? -> execute above command
All new and existing tests passed.

No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.

No? Unsure? -> execute command git pull --rebase upstream master

HerbertYiga · 2020-01-15T08:44:29Z

you can get the commits squashed here!!

dilanthas · 2020-01-15T10:17:42Z

api/src/main/java/org/openmrs/api/db/hibernate/HibernatePersonDAO.java

+		int maxResults = HibernatePersonDAO.getMaximumSearchResults();
+
+		LuceneQuery<PersonName> luceneQuery = personLuceneQuery.getSoundexPersonNameQuery(query, 0, true, "M");;
+		//getSoundexPersonNameQuery(query,birthyear, false, gender);


Please remove commented code

dilanthas · 2020-01-15T10:17:52Z

api/src/main/java/org/openmrs/api/db/hibernate/HibernatePersonDAO.java

+			temp_OutSource_Operation(name);
+			//q.append("(").append(" soundex(pname.givenName) = soundex(:n1)").append(
+			//    " or soundex(pname.middleName) = soundex(:n1)").append(" or soundex(pname.familyName) = soundex(:n1) ")
+			//        .append(" or soundex(pname.familyName2) = soundex(:n1) ").append(")");


Please remove commented code

dilanthas · 2020-01-15T10:18:06Z

api/src/main/java/org/openmrs/api/db/hibernate/PersonLuceneQuery.java

+			//(year(p.birthdate) between " + (birthyear - 1) + " and " + (birthyear + 1)
+			//	+ " or p.birthdate is null) 


Please remove commented code

dilanthas · 2020-01-15T10:18:42Z

api/src/main/java/org/openmrs/api/db/hibernate/PersonLuceneQuery.java

+		//	luceneQuery.include("voided", false);
+			//	luceneQuery.include("person.voided", false);


This if block is empty. You can remove it

dilanthas · 2020-01-15T10:18:52Z

api/src/main/java/org/openmrs/api/db/hibernate/PersonLuceneQuery.java

+
+		if(gender != null) {
+			String[] searchedGenders = new String[] {gender, ""};
+			//luceneQuery.include("person.gender", searchedGenders);


Please remove commented code

fruether · 2020-01-15T13:43:34Z

Hi, *@dilanthas * the PR only serves for the matter of discussion (it's not working). So I will come back to these comments once the real issue is fixed. Thanks for your effort. Link you discussion: https://talk.openmrs.org/t/soundex-search-in-lucenequeries/26068/13 @rkorytkowski any idea having a look to the current state of the code why the analyzer does not find any result? Dilantha Silva <notifications@github.com> schrieb am Mi., 15. Jan. 2020, 11:19:

…

***@***.**** requested changes on this pull request. ------------------------------ In api/src/main/java/org/openmrs/api/db/hibernate/HibernatePersonDAO.java <#3116 (comment)>: > @@ -72,6 +72,20 @@ public void setSessionFactory(SessionFactory sessionFactory) { this.sessionFactory = sessionFactory; } + + private ListPart<Object[]> temp_OutSource_Operation(String name) { + PersonLuceneQuery personLuceneQuery = new PersonLuceneQuery(sessionFactory); + String query = LuceneQuery.escapeQuery(name); + int maxResults = HibernatePersonDAO.getMaximumSearchResults(); + + LuceneQuery<PersonName> luceneQuery = personLuceneQuery.getSoundexPersonNameQuery(query, 0, true, "M");; + //getSoundexPersonNameQuery(query,birthyear, false, gender); Please remove commented code ------------------------------ In api/src/main/java/org/openmrs/api/db/hibernate/HibernatePersonDAO.java <#3116 (comment)>: > if (names.length == 1) { - q.append("(").append(" soundex(pname.givenName) = soundex(:n1)").append( - " or soundex(pname.middleName) = soundex(:n1)").append(" or soundex(pname.familyName) = soundex(:n1) ") - .append(" or soundex(pname.familyName2) = soundex(:n1) ").append(")"); + temp_OutSource_Operation(name); + //q.append("(").append(" soundex(pname.givenName) = soundex(:n1)").append( + // " or soundex(pname.middleName) = soundex(:n1)").append(" or soundex(pname.familyName) = soundex(:n1) ") + // .append(" or soundex(pname.familyName2) = soundex(:n1) ").append(")"); Please remove commented code ------------------------------ In api/src/main/java/org/openmrs/api/db/hibernate/PersonLuceneQuery.java <#3116 (comment)>: > + //(year(p.birthdate) between " + (birthyear - 1) + " and " + (birthyear + 1) + // + " or p.birthdate is null) Please remove commented code ------------------------------ In api/src/main/java/org/openmrs/api/db/hibernate/PersonLuceneQuery.java <#3116 (comment)>: > + // luceneQuery.include("voided", false); + // luceneQuery.include("person.voided", false); This if block is empty. You can remove it ------------------------------ In api/src/main/java/org/openmrs/api/db/hibernate/PersonLuceneQuery.java <#3116 (comment)>: > + // + " or p.birthdate is null) + String dateQuery = " AND person.birthdate: [" + (birthyear - 1) + " TO " + (birthyear + 1) + "]"; + completeQuery+= dateQuery; + } + + LuceneQuery<PersonName> luceneQuery = LuceneQuery + .newQuery(PersonName.class, sessionFactory.getCurrentSession(), query, fields).useOrQueryParser(); + + if (!includeVoided) { + // luceneQuery.include("voided", false); + // luceneQuery.include("person.voided", false); + } + + if(gender != null) { + String[] searchedGenders = new String[] {gender, ""}; + //luceneQuery.include("person.gender", searchedGenders); Please remove commented code — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3116?email_source=notifications&email_token=AAMEOAB32HFJU5E7TY2CNUTQ53PKBA5CNFSM4KGIAC6KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCRZYFBA#pullrequestreview-343114372>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMEOABKMUREDRIMZEKL23TQ53PKBANCNFSM4KGIAC6A> .

rkorytkowski · 2020-01-17T09:23:07Z

@fruether, I don't see anything obvious. I'll run your code once I have some spare cycles. Probably next week.

saadkhaleeq610 · 2020-01-19T17:03:56Z

Nice :) You can remove the comments.

jwnasambu · 2020-01-20T09:56:20Z

@saadkhaleeq610 why should the comment be removed? Comments are added with the purpose of making the source code easier for human to understand, and are generally ignored by compilers and interpreters. I anticipated you to talk about about the Travis CI failure in this case.

fruether · 2020-01-25T13:27:41Z

@jwnasambu this PR does not have the intend to be merged into the code base. It only has the reason to help the discussion in talk. That is why the "comments" can be ignored for now. They will be fixed once the code is working. Then I will do the beautification regarding adding source code comments, variable names, code squashing and so on.

Once I figured with the help of @rkorytkowski out what is missing, I will recreate the PR with . the previous comments taken into account :)

sanjulamadurapperuma · 2020-02-02T11:20:27Z

api/src/main/java/org/openmrs/api/db/hibernate/HibernatePersonDAO.java

@@ -84,6 +98,7 @@ public void setSessionFactory(SessionFactory sessionFactory) {
 		if (birthyear == null) {
 			birthyear = 0;
 		}
+


Remove unnecessary line here

sanjulamadurapperuma · 2020-02-02T11:21:45Z

api/src/test/java/org/openmrs/api/PersonServiceTest.java


+		//


This comment is empty, please remove it :)

sanjulamadurapperuma · 2020-02-02T11:22:19Z

api/src/main/java/org/openmrs/api/db/hibernate/search/LuceneAnalyzerFactory.java

+				.param("encoder", "Soundex")
+				.param("inject", "true");
+
+


Remove unnecessary blank line

@sumangala028 thanks for your review. But as I pointed out above: The code of this PR does not work. So we should focus on making the code work and then after that is established we can focus on more formal and beautification issues.

if you want to participate helping to make this code work: https://talk.openmrs.org/t/soundex-search-in-lucenequeries/26068/13

Otherwise I think the energy could be rather used on different reviews.

I see... thanks for pointing me to the discussion of this issue. Yes, the beautification issues can be pushed back till the code is fixed.

fruether · 2020-02-02T14:05:09Z

@fruether, I don't see anything obvious. I'll run your code once I have some spare cycles. Probably next week.

@rkorytkowski Is there a time slot during Februrary where you may have some spare time to run the code? Any hint what I can do/try out while waiting for feedback?

brandones · 2020-03-05T18:39:28Z

Hi @fruether , nice work on this. My recommendation would be to write some tests (parallel to the patient search test I wrote, which checks whether accent flattening works). I think that will help clarify the problem, and also provide a nice entry point for debugging.

coveralls · 2020-03-21T23:06:51Z

Coverage increased (+0.08%) to 60.28% when pulling 5e078fd on fruether:TRUNK-5680 into 9345129 on openmrs:master.

Adding Soundex Lucene Analyzer

acb1c7a

fruether force-pushed the TRUNK-5680 branch 9 times, most recently from 6759c09 to b90e31d Compare January 13, 2020 20:23

dilanthas suggested changes Jan 15, 2020

View reviewed changes

sanjulamadurapperuma reviewed Feb 2, 2020

View reviewed changes

api/src/test/java/org/openmrs/api/PersonServiceTest.java Outdated

//

Copy link

sanjulamadurapperuma Feb 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is empty, please remove it :)

sanjulamadurapperuma reviewed Feb 2, 2020

View reviewed changes

fruether changed the title ~~Trunk-5680 - Refactor getSimilarPeople to use Lucene~~ Trunk-5680 - For Debug reasons - Refactor getSimilarPeople to use Lucene Feb 2, 2020

fruether force-pushed the TRUNK-5680 branch 2 times, most recently from 051ec5c to d1ba85e Compare March 18, 2020 22:12

Trunk-5680 one and two names lucene soundex search

d4d8b6a

fruether force-pushed the TRUNK-5680 branch from d1ba85e to d4d8b6a Compare March 18, 2020 22:13

Implementing one and two name search

5e078fd

fruether closed this Mar 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trunk-5680 - For Debug reasons - Refactor getSimilarPeople to use Lucene #3116

Trunk-5680 - For Debug reasons - Refactor getSimilarPeople to use Lucene #3116

fruether commented Jan 13, 2020 •

edited

Loading

HerbertYiga commented Jan 15, 2020

dilanthas Jan 15, 2020

dilanthas Jan 15, 2020

dilanthas Jan 15, 2020

dilanthas Jan 15, 2020

dilanthas Jan 15, 2020

fruether commented Jan 15, 2020 via email •

edited

Loading

rkorytkowski commented Jan 17, 2020

saadkhaleeq610 commented Jan 19, 2020

jwnasambu commented Jan 20, 2020 •

edited

Loading

fruether commented Jan 25, 2020

sanjulamadurapperuma Feb 2, 2020

sanjulamadurapperuma Feb 2, 2020

sanjulamadurapperuma Feb 2, 2020

fruether Feb 2, 2020

sanjulamadurapperuma Feb 2, 2020

fruether commented Feb 2, 2020

brandones commented Mar 5, 2020

coveralls commented Mar 21, 2020

		//(year(p.birthdate) between " + (birthyear - 1) + " and " + (birthyear + 1)
		// + " or p.birthdate is null)

		// luceneQuery.include("voided", false);
		// luceneQuery.include("person.voided", false);

Trunk-5680 - For Debug reasons - Refactor getSimilarPeople to use Lucene #3116

Trunk-5680 - For Debug reasons - Refactor getSimilarPeople to use Lucene #3116

Conversation

fruether commented Jan 13, 2020 • edited Loading

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

HerbertYiga commented Jan 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fruether commented Jan 15, 2020 via email • edited Loading

rkorytkowski commented Jan 17, 2020

saadkhaleeq610 commented Jan 19, 2020

jwnasambu commented Jan 20, 2020 • edited Loading

fruether commented Jan 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fruether commented Feb 2, 2020

brandones commented Mar 5, 2020

coveralls commented Mar 21, 2020

fruether commented Jan 13, 2020 •

edited

Loading

fruether commented Jan 15, 2020 via email •

edited

Loading

jwnasambu commented Jan 20, 2020 •

edited

Loading