----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71736/#review218646 -----------------------------------------------------------
Le - the updated patch looks good! Please review the comments and update. Thanks! intg/src/main/java/org/apache/atlas/type/AtlasBuiltInTypes.java Lines 53 (patched) <https://reviews.apache.org/r/71736/#comment306470> getType() methods added in AtlasBooleanType/AtlasLongType/AtlasStringType don't seem to be used. Please review and remove if unused. intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java Lines 45 (patched) <https://reviews.apache.org/r/71736/#comment306471> - please move 'static' field to #42, before instance fields - consider renaming 'baseType' to 'CLASSIFICATION_ROOT' - consider marking this as 'public final' and initialize with: public static final AtlasClassificationType CLASSIFICATION_ROOT = initBaseClassificationType(); - remove assignment in #86, as this will be initialized in every call to the constructor intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java Lines 87 (patched) <https://reviews.apache.org/r/71736/#comment306474> Instead of calling baseType.resolveReferences() here i.e. in every instantiation of AtlasClassificationType, consider calling from AtlasTypeRegistry.resolveReferences(). intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java Lines 92 (patched) <https://reviews.apache.org/r/71736/#comment306472> - please move all 'private' methods to end of the class - per comment in #45, mark this method as 'static', so that this can be called to initialize 'static final' in #45. intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java Lines 95 (patched) <https://reviews.apache.org/r/71736/#comment306473> Following system-attributes should be added to classifications: - Constants.TIMESTAMP_PROPERTY_KEY - Constants.MODIFICATION_TIMESTAMP_PROPERTY_KEY - Constants.CREATED_BY_KEY - Constants.MODIFIED_BY_KEY intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java Lines 100 (patched) <https://reviews.apache.org/r/71736/#comment306475> ALL_CLASSIFIED => __CLASSIFICATION_ROOT intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java Lines 304 (patched) <https://reviews.apache.org/r/71736/#comment306476> Consider removing hasAttribute() and getAttribute() implementations in AtlasClassificationType and AtlasEntityType; and update AtlasStructType.getAttribute as: class AtlasStructType { ... public AtlasAttribute getAttribute(String attributeName) { AtlasAttribute ret = allAttributes.get(attributeName); if (ret == null) { ret = getSystemAttribute(attributeName); } return ret; } public AtlasAttribute getSystemAttribute(attributeName) { return null; } } intg/src/main/java/org/apache/atlas/type/AtlasEntityType.java Lines 650 (patched) <https://reviews.apache.org/r/71736/#comment306478> ALL_ENTITY_TYPES => __ENTITY_ROOT intg/src/main/java/org/apache/atlas/type/AtlasStructType.java Lines 50 (patched) <https://reviews.apache.org/r/71736/#comment306479> Struct-types are not needed in searches (yet). So, baseStructAttributeDef and SYSTEM_ATTRIBUTES here may not be necessary. Please review and remove. intg/src/main/java/org/apache/atlas/type/AtlasStructType.java Line 634 (original), 673 (patched) <https://reviews.apache.org/r/71736/#comment306480> AtlasAttribute attribute = getAttribute(attrName); if (attribute != null) { return attribute.getQualifiedName(); } throw new AtlasBaseException(AtlasErrorCode.UNKNOWN_ATTRIBUTE, attrName, structDef.getName()); intg/src/main/java/org/apache/atlas/type/AtlasStructType.java Lines 960 (patched) <https://reviews.apache.org/r/71736/#comment306481> if (StringUtils.equals(structDef.getName(), AtlasEntityType.ENTITY_ROOT.getName()) || StringUtils.equals(structDef.getName(), AtlasClassificationType.CLASSIFICATION_ROOT.getName()) || StringUtils.isEmpty(structDef.getName()) { return attrName; } else { ... } intg/src/main/java/org/apache/atlas/type/AtlasStructType.java Line 923 (original), 968 (patched) <https://reviews.apache.org/r/71736/#comment306482> String attrName = attrDef.getName(); if (StringUtils.equals(structDef.getName(), AtlasEntityType.ENTITY_ROOT.getName()) || StringUtils.equals(structDef.getName(), AtlasClassificationType.CLASSIFICATION_ROOT.getName()) || StringUtils.isEmpty(structDef.getName()) { return attrName; } else { String vertexPropertyName = qualifiedName; ... } intg/src/main/java/org/apache/atlas/type/Constants.java Lines 58 (patched) <https://reviews.apache.org/r/71736/#comment306483> Value of CLASSIFICATION_VERTEX_NAME_KEY is same as TYPE_NAME_PROPERTY_KEY. Consider removing CLASSIFICATION_VERTEX_NAME_KEY. repository/src/main/java/org/apache/atlas/discovery/ClassificationSearchProcessor.java Line 114 (original), 114 (patched) <https://reviews.apache.org/r/71736/#comment306484> is it necessary to add "!context.hasAttributeFilter(filterCriteria)" here? Shouldn't canApplyIndexFilter(classificationType, filterCriteria, false) be updated to handle this condition? repository/src/main/java/org/apache/atlas/discovery/EntitySearchProcessor.java Lines 108 (patched) <https://reviews.apache.org/r/71736/#comment306485> it might be efficient to set typeNamePredicate to null here, and updates the references to typeNamePredicate to handle null. repository/src/main/java/org/apache/atlas/discovery/SearchContext.java Lines 74 (patched) <https://reviews.apache.org/r/71736/#comment306491> Following are not updated after initial assignment in the constructor. Please review and retain 'final' marker: - classificationName - classificationType Also, consider retaining 'final' marker for the following, by moving contents of buildClassificationTypeAndSubTypes() and buildEntityTypeAndSubTypes() to constructor. - typeAndSubTypes - classificationTypeAndSubTypes - typeAndSubTypesQryStr - classificationTypeAndSubTypesQryStr repository/src/main/java/org/apache/atlas/discovery/SearchContext.java Line 85 (original), 91 (patched) <https://reviews.apache.org/r/71736/#comment306492> Instead of instantiating an AtlasEntityType object in every search, consider using AtlasEntityType.ENTITY_ROOT repository/src/main/java/org/apache/atlas/discovery/SearchContext.java Line 279 (original), 262 (patched) <https://reviews.apache.org/r/71736/#comment306489> structType.getAttributeType() should handles system-attributes as well. Changes in #262, #263 may not be necessary. Please review and update. repository/src/main/java/org/apache/atlas/discovery/SearchContext.java Line 288 (original), 272 (patched) <https://reviews.apache.org/r/71736/#comment306490> There are no derived classes for SearchContext. Is this access change from private to protected neceessary? Same for #285 as well. repository/src/main/java/org/apache/atlas/repository/graph/GraphBackedSearchIndexer.java Lines 515 (patched) <https://reviews.apache.org/r/71736/#comment306488> Only change in this file seems to be move of #501, #502 to #515, #516. If this move is not necessary, please consider reverting updates in this file. webapp/src/main/java/org/apache/atlas/web/rest/DiscoveryREST.java Line 332 (original), 328 (patched) <https://reviews.apache.org/r/71736/#comment306487> #328 - #330 should be removed to support search for classification-system-attributes. webapp/src/main/java/org/apache/atlas/web/rest/DiscoveryREST.java Lines 333 (patched) <https://reviews.apache.org/r/71736/#comment306486> Also add: "&& parameters.getTagFilters() == null" - Madhan Neethiraj On Nov. 14, 2019, 11:57 p.m., Le Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71736/ > ----------------------------------------------------------- > > (Updated Nov. 14, 2019, 11:57 p.m.) > > > Review request for atlas, Ashutosh Mestry, Aadarsh Jajodia, Sridhar K, Madhan > Neethiraj, and Sarath Subramanian. > > > Bugs: ATLAS-3482 > https://issues.apache.org/jira/browse/ATLAS-3482 > > > Repository: atlas > > > Description > ------- > > Introduce ALL_ENTITY_TYPE to support search on system attributes across all > entity types. System attributes will be passed in as normal entity attributes. > > > Diffs > ----- > > intg/src/main/java/org/apache/atlas/model/discovery/SearchParameters.java > 8f0e5912d > intg/src/main/java/org/apache/atlas/model/typedef/AtlasStructDef.java > e10965b87 > intg/src/main/java/org/apache/atlas/type/AtlasBuiltInTypes.java ed1e5ded2 > intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java > 417194202 > intg/src/main/java/org/apache/atlas/type/AtlasEntityType.java 884447f81 > intg/src/main/java/org/apache/atlas/type/AtlasStructType.java e8bf7f9eb > intg/src/main/java/org/apache/atlas/type/Constants.java PRE-CREATION > > repository/src/main/java/org/apache/atlas/discovery/ClassificationSearchProcessor.java > 672f38132 > > repository/src/main/java/org/apache/atlas/discovery/EntitySearchProcessor.java > 03eb92bcc > > repository/src/main/java/org/apache/atlas/discovery/FullTextSearchProcessor.java > 152ade8d4 > repository/src/main/java/org/apache/atlas/discovery/SearchContext.java > 7ad32bdb9 > repository/src/main/java/org/apache/atlas/discovery/SearchProcessor.java > b56d8e83a > > repository/src/main/java/org/apache/atlas/repository/graph/GraphBackedSearchIndexer.java > 7c551304b > webapp/src/main/java/org/apache/atlas/web/rest/DiscoveryREST.java 825cda30b > webapp/src/test/java/org/apache/atlas/web/adapters/TestEntitiesREST.java > cd8f8981c > > > Diff: https://reviews.apache.org/r/71736/diff/3/ > > > Testing > ------- > > - Added new unit tests, passed. > - will run pre-commit job. > > > Thanks, > > Le Ma > >
