-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72713/#review221419
-----------------------------------------------------------




repository/src/main/java/org/apache/atlas/discovery/SearchProcessor.java
Lines 634 (patched)
<https://reviews.apache.org/r/72713/#comment310402>

    localDate => now



repository/src/main/java/org/apache/atlas/discovery/SearchProcessor.java
Lines 635 (patched)
<https://reviews.apache.org/r/72713/#comment310403>

    Is 'ts' instance needed? only use is to call method valueOf(), which is a 
static method. I suggest to remove 'ts', and replace the calls with 
"Timestamp.valueOf()"



repository/src/main/java/org/apache/atlas/discovery/SearchProcessor.java
Lines 642 (patched)
<https://reviews.apache.org/r/72713/#comment310405>

    The end time should be 'end of today'; not the current time. I suggest the 
following:
    
     final LocalDateTime now = LocalDateTime.now();
     final LocalDateTime startTime;
     final LocalDateTime endTime;
    
     switch (attrVal) {
       case "LAST_7_DAYS":
         startTime = 
now.minusDays(6).withHour(0).withMinute(0).withSecond(0).withNano(0);
         endTime   = startTime.plusDays(7).minusNanos(1);
       break;
    
       case "LAST_30_DAYS":
         startTime = 
now.minusDays(29).withHour(0).withMinute(0).withSecond(0).withNano(0);
         endTime   = startTime.plusDays(30).minusNanos(1);
       break;
    
       case "LAST_MONTH":
         startTime = 
now.minusMonths(1).withDayOfMonth(0).withHour(0).withMinute(0).withSecond(0).withNano(0);
         endTime   = startTime.plusMonths(1).minusNanos(1);
       break;
    
       case "THIS_MONTH":
         startTime = 
now.withDayOfMonth(1).withHour(0).withMinute(0).withSecond(0).withNano(0);
         endTime   = startTime.plusMonths(1).minusNanos(1);
       break;
    
       case "TODAY":
         startTime = now.withHour(0).withMinute(0).withSecond(0).withNano(0);
         endTime   = startTime.plusDays(1).minusNanos(1);
       break;
    
       case "YESTERDAY":
         startTime = 
now.minusDays(1).withHour(0).withMinute(0).withSecond(0).withNano(0);
         endTime   = startTime.plusDays(1).minusNanos(1);
       break;
    
       default:
         startTime = null;
         endTime   = null;
       break;
     }
     
     if (startTime == null || endTime == null) {
       // parse attrVal and update
     } else {
       attrVal = Timestamp.valueOf(startTime) + ATTRIBUTE_VALUE_DELIMITER + 
Timestamp.valueOf(endTime)
     }



repository/src/main/java/org/apache/atlas/discovery/SearchProcessor.java
Lines 739 (patched)
<https://reviews.apache.org/r/72713/#comment310393>

    rangeStart and rangeEnd are referenced only inside the block at #747. 
Please move #739, #740 inside that block.



repository/src/main/java/org/apache/atlas/discovery/SearchProcessor.java
Lines 754 (patched)
<https://reviews.apache.org/r/72713/#comment310394>

    Throw AtlasBaseException here when attrVal doesn't have expected value. 
This will be handled at #767 (prints a warn log)



repository/src/main/java/org/apache/atlas/discovery/SearchProcessor.java
Lines 848 (patched)
<https://reviews.apache.org/r/72713/#comment310395>

    Even for single-line blocks, use { } - for consistency.



repository/src/main/java/org/apache/atlas/util/SearchPredicateUtil.java
Lines 192 (patched)
<https://reviews.apache.org/r/72713/#comment310396>

    add @Override annotation here as well



repository/src/main/java/org/apache/atlas/util/SearchPredicateUtil.java
Lines 207 (patched)
<https://reviews.apache.org/r/72713/#comment310399>

    "<-" -> "<=="



repository/src/main/java/org/apache/atlas/util/SearchPredicateUtil.java
Lines 804 (patched)
<https://reviews.apache.org/r/72713/#comment310397>

    Wouldn't this cause infinite recursion, as this calls itself? Shouldn't 
this call the existing method at #801 i.e. method that takes a single attrVal?



repository/src/main/java/org/apache/atlas/util/SearchPredicateUtil.java
Line 977 (original), 1019 (patched)
<https://reviews.apache.org/r/72713/#comment310398>

    This change to constructor signature doesn't seem necessary. Please revert. 
Also, note that the new constructor at #1025 has same arguments, but in 
different order.



repository/src/test/java/org/apache/atlas/discovery/EntitySearchProcessorTest.java
Lines 319 (patched)
<https://reviews.apache.org/r/72713/#comment310400>

    mark filtercriteriaDateRange() as private and static; move to end of this 
class.



repository/src/test/java/org/apache/atlas/discovery/EntitySearchProcessorTest.java
Lines 329 (patched)
<https://reviews.apache.org/r/72713/#comment310401>

    attributeName is not set in filterCriteria? For completeness, please set to 
'createTime'. Also, consider populating filterCriteria before adding to params 
in #323.


- Madhan Neethiraj


On July 30, 2020, 2:34 p.m., chaitali wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72713/
> -----------------------------------------------------------
> 
> (Updated July 30, 2020, 2:34 p.m.)
> 
> 
> Review request for atlas, Jayendra Parab, Madhan Neethiraj, Nixon Rodrigues, 
> and Sarath Subramanian.
> 
> 
> Bugs: ATLAS-3892
>     https://issues.apache.org/jira/browse/ATLAS-3892
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> 1.This patch implements the search history feature where from UI we will 
> receive below filters.
> 2.In backend we have introduced a new operator "TIME_RANGE" where all the 
> below 7 attribute values will have TIME_RANGE operator.
> 3.There will be a slight change in the JSON where previously we were passing 
> the Attribute value in Search parameters from the UI but for below filters   
> we will not pass any attributeValue except for "custom range" filter we 
> provide attribute value 
>   for eg ;"attributeValue":"1593541800000,1595442600000" .
> 
> New 7 filters :
> 
> 1.last 7 days.
> 2.Last month.
> 3.Last 30 days.
> 4.Today.
> 5.Yesterday.
> 6.Custom Range.
> 7.This Month.
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/atlas/repository/Constants.java a71787bc0 
>   intg/src/main/java/org/apache/atlas/model/discovery/SearchParameters.java 
> fcc4494f2 
>   repository/src/main/java/org/apache/atlas/discovery/SearchProcessor.java 
> c9a605355 
>   repository/src/main/java/org/apache/atlas/util/SearchPredicateUtil.java 
> 5069d78c8 
>   
> repository/src/test/java/org/apache/atlas/discovery/EntitySearchProcessorTest.java
>  8e42d1700 
> 
> 
> Diff: https://reviews.apache.org/r/72713/diff/5/
> 
> 
> Testing
> -------
> 
> Tested with below json: 
> eg : 
> {
>   "excludeDeletedEntities": true,
>   "includeSubClassifications": true,
>   "includeSubTypes": true,
>   "includeClassificationAttributes": true,
>   "entityFilters": {
>     "condition": "AND",
>     "criterion": [
>       {
>         "attributeName": "__timestamp",
>               "operator": "TIME_RANGE",
>               "attributeValue": "YESTERDAY"
>       }
>     ]
>   },
>   "tagFilters": null,
>   "attributes": [
>     "__timestamp"
>   ],
>   "limit": 25,
>   "offset": 0,
>   "typeName": "hdfs_path",
>   "classification": null,
>   "termName": null
> }
> eg: Only For custom range filter we provide attribute value.
> {
>   "excludeDeletedEntities": true,
>   "includeSubClassifications": true,
>   "includeSubTypes": true,
>   "includeClassificationAttributes": true,
>   "entityFilters": {
>     "condition": "AND",
>     "criterion": [
>       {
>         "attributeName": "__timestamp",
>               "operator": "TIME_RANGE",
>               "attributeValue": "1595956315473,1595961000000"
>       }
>     ]
>   },
>   "tagFilters": null,
>   "attributes": [
>     "__timestamp"
>   ],
>   "limit": 25,
>   "offset": 0,
>   "typeName": "Table",
>   "classification": null,
>   "termName": null
> }
> 
> * Added Test cases to check new processDateRange method functionality
> 
> 
> Thanks,
> 
> chaitali
> 
>

Reply via email to