Fixing issue #9 #48
Fixing issue #9 #48
Conversation
konstjar
requested review from
dmitrys-odysseus
and removed request for
wivern
| (isSubmittedSort(pageable) ?"submitted": | ||
| (isStatusSort(pageable)?"status":"")); | ||
| if(pageable.getSort() == null || "".equals(sortField)) | ||
| { |
There was a problem hiding this comment.
Defect: Formatting. Please stick to existing de facto formatting style, specifically:
- consistent spacing (e.g. single space) around operators and keywords, after comma
- opening brace on the same line
- comma not dropped to the next line
While minor formatting issues might be overlooked, the current state is almost unreadable.
There was a problem hiding this comment.
This issue is still not addressed, while he code was removed, similar problems exist in newly added code
datanode/src/main/java/com/odysseusinc/arachne/datanode/repository/AnalysisRepository.java
Outdated
Show resolved
Hide resolved
| " ELSE CAST(a.id as text)\n" + | ||
| " END \n" + | ||
| " END DESC", | ||
| countQuery = |
There was a problem hiding this comment.
The separate countQuery is unnecessary and is very bad for maintainability. We can use the FROM and WHERE for both queries and just alternate the prefix.
There was a problem hiding this comment.
when we use the paging and sorting of calculated fields with spring data it does not return the information as expected, that is why we use native queries combined with spring data paging, if I do the paging and sorting without using spring data, I would have to do countQuery separately to supply that information to the Page object that is being returned.
| " END ASC,\n" + | ||
| " CASE WHEN :direction = 'DESC' THEN\n" + | ||
| " CASE :sortField \n" + | ||
| " WHEN 'submitted' THEN CAST(COALESCE(JS.submitted, CAST('epoch' as timestamp)) as text)\n" + |
There was a problem hiding this comment.
If there are any differences between these 2 blocks, this is "Defect: ASC and DESC sort order are in consistent"
otherwise,it is "Defect: code duplication"
In either case, there is a defect.
There was a problem hiding this comment.
sorting is being done in the native query because when sorting by calculated field spring data does not return the data correctly, this change is only being done for those calculated fields.
| " ELSE CAST(a.id as text)\n" + | ||
| " END \n" + | ||
| " END ASC,\n" + | ||
| " CASE WHEN :direction = 'DESC' THEN\n" + |
There was a problem hiding this comment.
Why do you CASE over fields that is statically available during query construction?
There is no rule that prohibits you from building SQL via code rather than using spring data.
The only measure of what is better is complexity and the current SQL is already too complex.
| " ORDER BY \n" + | ||
| " CASE WHEN :direction = 'ASC' THEN\n" + | ||
| " CASE :sortField \n" + | ||
| " WHEN 'submitted' THEN CAST(COALESCE(JS.submitted, CAST('epoch' as timestamp)) as text)\n" + |
There was a problem hiding this comment.
CAST('epoch' as timestamp) ?
Why is this?
What is epoch?
There was a problem hiding this comment.
query was changed optimizing this change
| " ,JS.submitted \n" + | ||
| " ,JS.finished\n" + | ||
| " ,JS.status\n" + | ||
| " ,SV.id\n" + |
There was a problem hiding this comment.
SV.id? a also has id, so I think this is a conflict.
There was a problem hiding this comment.
query was changed optimizing this change
| " else j.date \n" + | ||
| " end \n" + | ||
| " ) over(partition by j.analysis_id) as finished,\n" + | ||
| " FIRST_VALUE(j.state) over(partition by j.analysis_id order by j.date desc) status\n" + |
There was a problem hiding this comment.
We strive to keep any SQL compliant to the lowest reasonable standard (down to SQL2003).
A partition over window window function is something far beoynd that, so it has to be well-justified.
Since we are in subquery already, I think a trivial WHERE j.analysis_id=a.id would suffice.
General note: In most cases, you need either subquerty or partition over window function. A situation that justifies using both has to be much more complicated than what we have here.
There was a problem hiding this comment.
query was changed optimizing this change
…r calculate fields in AnalysisRepository
| super(userService); | ||
| initProps(); | ||
| } | ||
| public static final int SUGGEST_LIMIT = 10; |
There was a problem hiding this comment.
Please revert unnecessary formatting changes. For java code is de facto standard is 4 spaces, why are you putting in 2?
| }); | ||
| propertiesMap.put("analysis", p -> p.add("title")); | ||
| propertiesMap.put("study", p -> p.add("studyTitle")); | ||
| propertiesMap.put("status", p -> p.add("journal.state")); |
There was a problem hiding this comment.
Why is this line removed?
| (isSubmittedSort(pageable) ?"submitted": | ||
| (isStatusSort(pageable)?"status":"")); | ||
| if(pageable.getSort() == null || "".equals(sortField)) | ||
| { |
There was a problem hiding this comment.
This issue is still not addressed, while he code was removed, similar problems exist in newly added code
| sqlBuilder.append("CASE WHEN J.state IN('EXECUTING','CREATED') ") | ||
| .append("THEN null ") | ||
| .append("ELSE JS.finished END "); | ||
| }else if(sortField!=null && sortField.equals("status")) |
There was a problem hiding this comment.
Defect: code duplication. (sortField != null) only needs to be checked once, checking for specific values can go into the nested block
| .append("MIN(J.date) AS submitted ") | ||
| .append("FROM analysis_state_journal J ") | ||
| .append("GROUP BY J.analysis_id) JS ON J.analysis_id=JS.analysis_id AND J.date = JS.finished ") | ||
| .append("ORDER BY "); |
There was a problem hiding this comment.
When sortField == null ORDER BY is still added. Is there a reason for that?
| List<Analysis> content =query.getResultList(); | ||
|
|
||
| countBuilder = | ||
| (sortField != null && (sortField.equals("finished") || sortField.equals("status"))) ? |
There was a problem hiding this comment.
Why does count query depend on sortField?
| countBuilder = | ||
| (sortField != null && (sortField.equals("finished") || sortField.equals("status"))) ? | ||
| countBuilder.append("select count(a.*) from analyses a ") | ||
| .append("JOIN analysis_state_journal AS journal ON journal.analysis_id = a.id ") |
There was a problem hiding this comment.
Normally, you want to reuse the exact same FROM and WHERE clauses for count query to make sure that the count matched the query. You are not really doing it here and clauses are slightly different. Why?
| StringBuilder countBuilder = new StringBuilder(); | ||
|
|
||
| sqlBuilder.append("SELECT a.* FROM analyses a ") | ||
| .append("JOIN analysis_state_journal AS J ON J.analysis_id = a.id ") |
There was a problem hiding this comment.
Reading .append( on each line doesn't help with understanding, polluting the view.
Java compiler optimizes string concatenation via "+" replacing it with StringBuilder automatically since JDK 1.6 (since 2006!), so unless concatenation happens in a loop, explicit usage of StringBuilder yields zero performance benefit.
| .append("MAX(J.date) AS finished, ") | ||
| .append("MIN(J.date) AS submitted ") | ||
| .append("FROM analysis_state_journal J ") | ||
| .append("GROUP BY J.analysis_id) JS ON J.analysis_id=JS.analysis_id AND J.date = JS.finished ") |
There was a problem hiding this comment.
I had to use Ctrl-F to find that closing brace on the join. When breaking the lines, "prefer to break at a higher syntactic level". Or just group the fragments logically extracting variables:
String select = ...
String from = ...
String orderBy = ...
String query = "SELECT a.*" + from + orderBy;
...
String countsQuery = "SELECT count(a.*) + from;
No description provided.