----------------------------------------------------------- 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 > >
