ATLAS-2354: Null/empty handling for attributes specified with select

repository/src/main/java/org/apache/atlas/query/DSLVisitor.java

@@ -226,6 +226,9 @@ public class DSLVisitor extends AtlasDSLParserBaseVisitor<Void> {
         processExpr(expr.compE(), nestedProcessor);
         nestedQueries.add(nestedProcessor.get());
 
+        // Record all processed attributes
+        gremlinQueryComposer.addProcessedAttributes(nestedProcessor.getAttributesProcessed());
+
         for (ExprRightContext exprRight : expr.exprRight()) {
             nestedProcessor = gremlinQueryComposer.createNestedProcessor();
 
@@ -238,6 +241,9 @@ public class DSLVisitor extends AtlasDSLParserBaseVisitor<Void> {
                     orClause.addOrClauses(nestedQueries);
                     nestedQueries.clear();
                     nestedQueries.add(orClause.get());
+
+                    // Record all processed attributes
+                    gremlinQueryComposer.addProcessedAttributes(orClause.getAttributesProcessed());
                 }
                 prev = AND;
             }
@@ -250,11 +256,17 @@ public class DSLVisitor extends AtlasDSLParserBaseVisitor<Void> {
                     andClause.addAndClauses(nestedQueries);
                     nestedQueries.clear();
                     nestedQueries.add(andClause.get());
+
+                    // Record all processed attributes
+                    gremlinQueryComposer.addProcessedAttributes(andClause.getAttributesProcessed());
                 }
                 prev = OR;
             }
             processExpr(exprRight.compE(), nestedProcessor);
             nestedQueries.add(nestedProcessor.get());
+
+            // Record all processed attributes
+            gremlinQueryComposer.addProcessedAttributes(nestedProcessor.getAttributesProcessed());
         }
         if (AND.equalsIgnoreCase(prev)) {
             gremlinQueryComposer.addAndClauses(nestedQueries);

repository/src/main/java/org/apache/atlas/query/GremlinQueryComposer.java

@@ -27,6 +27,7 @@ import org.apache.atlas.type.AtlasEntityType;
 import org.apache.atlas.type.AtlasStructType;
 import org.apache.atlas.type.AtlasType;
 import org.apache.atlas.type.AtlasTypeRegistry;
+import org.apache.commons.collections.CollectionUtils;
 import org.apache.commons.lang.StringUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -34,7 +35,14 @@ import org.slf4j.LoggerFactory;
 import java.text.DateFormat;
 import java.text.ParseException;
 import java.text.SimpleDateFormat;
-import java.util.*;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.TimeZone;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
@@ -46,7 +54,8 @@ public class GremlinQueryComposer {
     private final int DEFAULT_QUERY_RESULT_LIMIT = 25;
     private final int DEFAULT_QUERY_RESULT_OFFSET = 0;
 
-    private final GremlinClauseList      queryClauses   = new GremlinClauseList();
+
+    private final GremlinClauseList      queryClauses   = new GremlinClauseList();private final Set<String>       attributesProcessed = new HashSet<>();
     private final Lookup                 lookup;
     private final boolean                isNestedQuery;
     private final AtlasDSL.QueryMetadata queryMetadata;
@@ -164,6 +173,8 @@ public class GremlinQueryComposer {
         } else {
             add(GremlinClause.HAS_OPERATOR, lhsI.getQualifiedName(), op.getSymbols()[1], rhs);
         }
+        // record that the attribute has been processed so that the select clause doesn't add a attr presence check
+        attributesProcessed.add(lhsI.getQualifiedName());
 
         if (org != null && org.isReferredType()) {
             add(GremlinClause.DEDUP);
@@ -185,14 +196,51 @@ public class GremlinQueryComposer {
         add(GremlinClause.OR, String.join(",", clauses));
     }
 
+    public Set<String> getAttributesProcessed() {
+        return attributesProcessed;
+    }
+
+    public void addProcessedAttributes(Set<String> attributesProcessed) {
+        this.attributesProcessed.addAll(attributesProcessed);
+    }
+
+    public void addProcessedAttribute(String attribute) {
+        attributesProcessed.add(attribute);
+    }
+
     public void addSelect(SelectClauseComposer selectClauseComposer) {
         process(selectClauseComposer);
+
+        if (CollectionUtils.isEmpty(context.getErrorList())) {
+            addSelectAttrExistsCheck(selectClauseComposer);
+        }
+
+
+        // If the query contains orderBy and groupBy then the transformation determination is deferred to the method processing orderBy
         if (!(queryMetadata.hasOrderBy() && queryMetadata.hasGroupBy())) {
             addSelectTransformation(selectClauseComposer, null, false);
         }
         this.context.setSelectClauseComposer(selectClauseComposer);
     }
 
+    private void addSelectAttrExistsCheck(final SelectClauseComposer selectClauseComposer) {
+        // For each of the select attributes we need to add a presence check as well, if there's no explicit where for the same
+        // NOTE: One side-effect is that the result table will be empty if any of the attributes is null or empty for the type
+        String[] qualifiedAttributes = selectClauseComposer.getAttributes();
+        if (qualifiedAttributes != null && qualifiedAttributes.length > 0) {
+            for (int i = 0; i < qualifiedAttributes.length; i++) {
+                String                              qualifiedAttribute = qualifiedAttributes[i];
+                IdentifierHelper.IdentifierMetadata idMetadata         = getIdMetadata(qualifiedAttribute);
+                // Only primitive attributes need to be checked
+                if (idMetadata.isPrimitive() && !selectClauseComposer.isAggregatorIdx(i) && !attributesProcessed.contains(qualifiedAttribute)) {
+                    add(GremlinClause.HAS_PROPERTY, qualifiedAttribute);
+                }
+            }
+            // All these checks should be done before the grouping happens (if any)
+            moveToLast(GremlinClause.GROUP_BY);
+        }
+    }
+
     private void process(SelectClauseComposer scc) {
         if (LOG.isDebugEnabled()) {
             LOG.debug("addSelect(items.length={})", scc.getItems() != null ? scc.getItems().length : 0);

repository/src/main/java/org/apache/atlas/query/SelectClauseComposer.java

@@ -202,6 +202,10 @@ class SelectClauseComposer {
         aggCount++;
     }
 
+    public boolean isAggregatorIdx(int idx) {
+        return getMinIdx() == idx || getMaxIdx() == idx || getCountIdx() == idx || getSumIdx() == idx;
+    }
+
     private String getJoinedQuotedStr(String[] elements) {
         StringJoiner joiner = new StringJoiner(",");
         Arrays.stream(elements)

repository/src/test/java/org/apache/atlas/query/GremlinQueryComposerTest.java

@@ -71,7 +71,7 @@ public class GremlinQueryComposerTest {
     @Test
     public void DBasDSelect() {
         String expected = "def f(r){ t=[['d.name','d.owner']];  r.each({t.add([it.value('DB.name'),it.value('DB.owner')])}); t.unique(); }; " +
-                                  "f(g.V().has('__typeName', 'DB').as('d')";
+                                  "f(g.V().has('__typeName', 'DB').as('d').has('DB.name').has('DB.owner')";
         verify("DB as d select d.name, d.owner", expected + ".limit(local, 25).limit(25).toList())");
         verify("DB as d select d.name, d.owner limit 10", expected + ".limit(local, 10).limit(10).toList())");
         verify("DB as d select d","def f(r){ r }; f(g.V().has('__typeName', 'DB').as('d').limit(local, 25).limit(25).toList())");
@@ -111,7 +111,7 @@ public class GremlinQueryComposerTest {
                 "def f(l){ h=[['name','owner','clusterName']]; t=[]; " +
                         "l.get(0).each({k,r -> L:{  r.each({t.add([it.value('Table.name'),it.value('Table.owner'),it.value('Table.clusterName')])}) } }); " +
                         "h.plus(t.unique().sort{a,b -> a[0] <=> b[0]}); }; " +
-                        "f(g.V().has('__typeName', 'Table').group().by('Table.owner').limit(local, 25).limit(25).toList())");
+                        "f(g.V().has('__typeName', 'Table').has('Table.name').has('Table.owner').has('Table.clusterName').group().by('Table.owner').limit(local, 25).limit(25).toList())");
     }
 
     @Test
@@ -129,7 +129,7 @@ public class GremlinQueryComposerTest {
         verify("DB as d orderby d.owner limit 3", "g.V().has('__typeName', 'DB').as('d').order().by('DB.owner').limit(local, 3).limit(3).toList()");
 
         String exSel = "def f(r){ t=[['d.name','d.owner']];  r.each({t.add([it.value('DB.name'),it.value('DB.owner')])}); t.unique(); }";
-        String exMain = "g.V().has('__typeName', 'DB').as('d').order().by('DB.owner').limit(local, 25).limit(25).toList()";
+        String exMain = "g.V().has('__typeName', 'DB').as('d').has('DB.name').has('DB.owner').order().by('DB.owner').limit(local, 25).limit(25).toList()";
         verify("DB as d select d.name, d.owner orderby (d.owner) limit 25", getExpected(exSel, exMain));
 
         String exMain2 = "g.V().has('__typeName', 'Table').and(__.has('Table.name', eq(\"sales_fact\")),__.has('Table.createTime', gt('1418265300000'))).order().by('Table.createTime').limit(local, 25).limit(25).toList()";
@@ -145,7 +145,8 @@ public class GremlinQueryComposerTest {
 
     @Test
     public void fromDBSelect() {
-        String expected = "def f(r){ t=[['DB.name','DB.owner']];  r.each({t.add([it.value('DB.name'),it.value('DB.owner')])}); t.unique(); }; f(g.V().has('__typeName', 'DB').limit(local, 25).limit(25).toList())";
+        String expected = "def f(r){ t=[['DB.name','DB.owner']];  r.each({t.add([it.value('DB.name'),it.value('DB.owner')])}); t.unique(); }; " +
+                                  "f(g.V().has('__typeName', 'DB').has('DB.name').has('DB.owner').limit(local, 25).limit(25).toList())";
         verify("from DB select DB.name, DB.owner", expected);
     }
 
@@ -156,7 +157,7 @@ public class GremlinQueryComposerTest {
 
     @Test
     public void whereClauseTextContains() {
-        String exMain = "g.V().has('__typeName', 'DB').has('DB.name', eq(\"Reporting\")).limit(local, 25).limit(25).toList()";
+        String exMain = "g.V().has('__typeName', 'DB').has('DB.name', eq(\"Reporting\")).has('DB.owner').limit(local, 25).limit(25).toList()";
         String exSel = "def f(r){ t=[['name','owner']];  r.each({t.add([it.value('DB.name'),it.value('DB.owner')])}); t.unique(); }";
         verify("from DB where name = \"Reporting\" select name, owner", getExpected(exSel, exMain));
         verify("from DB where (name = \"Reporting\") select name, owner", getExpected(exSel, exMain));
@@ -169,20 +170,20 @@ public class GremlinQueryComposerTest {
     @Test
     public void whereClauseWithAsTextContains() {
         String exSel = "def f(r){ t=[['t.name','t.owner']];  r.each({t.add([it.value('Table.name'),it.value('Table.owner')])}); t.unique(); }";
-        String exMain = "g.V().has('__typeName', 'Table').as('t').has('Table.name', eq(\"testtable_1\")).limit(local, 25).limit(25).toList()";
+        String exMain = "g.V().has('__typeName', 'Table').as('t').has('Table.name', eq(\"testtable_1\")).has('Table.owner').limit(local, 25).limit(25).toList()";
         verify("Table as t where t.name = \"testtable_1\" select t.name, t.owner", getExpected(exSel, exMain));
     }
 
     @Test
     public void whereClauseWithDateCompare() {
         String exSel = "def f(r){ t=[['t.name','t.owner']];  r.each({t.add([it.value('Table.name'),it.value('Table.owner')])}); t.unique(); }";
-        String exMain = "g.V().has('__typeName', 'Table').as('t').has('Table.createTime', eq('1513046158440')).limit(local, 25).limit(25).toList()";
+        String exMain = "g.V().has('__typeName', 'Table').as('t').has('Table.createTime', eq('1513046158440')).has('Table.name').has('Table.owner').limit(local, 25).limit(25).toList()";
         verify("Table as t where t.createTime = \"2017-12-12T02:35:58.440Z\" select t.name, t.owner", getExpected(exSel, exMain));
     }
 
     @Test
     public void subType() {
-        String exMain = "g.V().has('__typeName', within('Asset','Table')).limit(local, 25).limit(25).toList()";
+        String exMain = "g.V().has('__typeName', within('Asset','Table')).has('Asset.name').has('Asset.owner').limit(local, 25).limit(25).toList()";
         String exSel = "def f(r){ t=[['name','owner']];  r.each({t.add([it.value('Asset.name'),it.value('Asset.owner')])}); t.unique(); }";
 
         verify("Asset select name, owner", getExpected(exSel, exMain));
@@ -307,7 +308,7 @@ public class GremlinQueryComposerTest {
     @Test
     public void systemAttributes() {
         verify("Table has __state", "g.V().has('__typeName', 'Table').has('__state').limit(local, 25).limit(25).toList()");
-        verify("Table select __guid", "def f(r){ t=[['__guid']];  r.each({t.add([it.value('__guid')])}); t.unique(); }; f(g.V().has('__typeName', 'Table').limit(local, 25).limit(25).toList())");
+        verify("Table select __guid", "def f(r){ t=[['__guid']];  r.each({t.add([it.value('__guid')])}); t.unique(); }; f(g.V().has('__typeName', 'Table').has('__guid').limit(local, 25).limit(25).toList())");
         verify("Table as t where t.__state = 'ACTIVE'", "g.V().has('__typeName', 'Table').as('t').has('__state', eq('ACTIVE')).limit(local, 25).limit(25).toList()");
     }