update checks in BQAttributeFilterTranslator

DataBiosphere · Feb 3, 2025 · 096e842 · 096e842
1 parent d357468
commit 096e842
Show file tree

Hide file tree

Showing 11 changed files with 141 additions and 52 deletions.
diff --git a/underlay/src/main/java/bio/terra/tanagra/api/filter/EntityFilter.java b/underlay/src/main/java/bio/terra/tanagra/api/filter/EntityFilter.java
@@ -40,6 +40,7 @@ public List<String> getFilterAttributeNames() {
   }
 
   // TODO: Add logic here to merge filters automatically to get a simpler filter overall.
+  // Current filter generation logic is in both UI and Underlay, hence only merge translation
   public boolean isMergeable(EntityFilter entityFilter) {
     return false;
   }
@@ -49,9 +50,15 @@ public EntityFilter mergeWith(EntityFilter entityFilter) {
     return null;
   }
 
-  public static boolean areSameFilterType(List<EntityFilter> filters) {
+  public static boolean areSameFilterTypeAndEntity(List<EntityFilter> filters) {
     Class<?> firstClazz = filters.get(0).getClass();
-    return filters.stream().skip(1).allMatch(filter -> filter.getClass().equals(firstClazz));
+    String firstEntityName = filters.get(0).getEntity().getName();
+    return filters.stream()
+        .skip(1)
+        .allMatch(
+            filter ->
+                filter.getClass().equals(firstClazz)
+                    && filter.getEntity().getName().equals(firstEntityName));
   }
 
   @Override

diff --git a/underlay/src/main/java/bio/terra/tanagra/api/filter/HierarchyHasParentFilter.java b/underlay/src/main/java/bio/terra/tanagra/api/filter/HierarchyHasParentFilter.java
@@ -6,6 +6,7 @@
 import bio.terra.tanagra.underlay.entitymodel.Hierarchy;
 import com.google.common.collect.ImmutableList;
 import java.util.List;
+import java.util.Objects;
 import org.slf4j.LoggerFactory;
 
 public class HierarchyHasParentFilter extends EntityFilter {
@@ -33,4 +34,18 @@ public Hierarchy getHierarchy() {
   public ImmutableList<Literal> getParentIds() {
     return parentIds;
   }
+
+  @Override
+  public boolean equals(Object o) {
+    if (!super.equals(o)) {
+      return false;
+    }
+    HierarchyHasParentFilter that = (HierarchyHasParentFilter) o;
+    return hierarchy.equals(that.hierarchy) && parentIds.equals(that.parentIds);
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(super.hashCode(), hierarchy, parentIds);
+  }
 }
diff --git a/underlay/src/main/java/bio/terra/tanagra/api/filter/HierarchyIsLeafFilter.java b/underlay/src/main/java/bio/terra/tanagra/api/filter/HierarchyIsLeafFilter.java
@@ -3,6 +3,7 @@
 import bio.terra.tanagra.underlay.Underlay;
 import bio.terra.tanagra.underlay.entitymodel.Entity;
 import bio.terra.tanagra.underlay.entitymodel.Hierarchy;
+import java.util.Objects;
 import org.slf4j.LoggerFactory;
 
 public class HierarchyIsLeafFilter extends EntityFilter {
@@ -16,4 +17,17 @@ public HierarchyIsLeafFilter(Underlay underlay, Entity entity, Hierarchy hierarc
   public Hierarchy getHierarchy() {
     return hierarchy;
   }
+
+  @Override
+  public boolean equals(Object o) {
+    if (!super.equals(o)) {
+      return false;
+    }
+    return hierarchy.equals(((HierarchyIsLeafFilter) o).hierarchy);
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(super.hashCode(), hierarchy);
+  }
 }
diff --git a/underlay/src/main/java/bio/terra/tanagra/api/filter/HierarchyIsMemberFilter.java b/underlay/src/main/java/bio/terra/tanagra/api/filter/HierarchyIsMemberFilter.java
@@ -3,6 +3,7 @@
 import bio.terra.tanagra.underlay.Underlay;
 import bio.terra.tanagra.underlay.entitymodel.Entity;
 import bio.terra.tanagra.underlay.entitymodel.Hierarchy;
+import java.util.Objects;
 import org.slf4j.LoggerFactory;
 
 public class HierarchyIsMemberFilter extends EntityFilter {
@@ -16,4 +17,17 @@ public HierarchyIsMemberFilter(Underlay underlay, Entity entity, Hierarchy hiera
   public Hierarchy getHierarchy() {
     return hierarchy;
   }
+
+  @Override
+  public boolean equals(Object o) {
+    if (!super.equals(o)) {
+      return false;
+    }
+    return hierarchy.equals(((HierarchyIsMemberFilter) o).hierarchy);
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(super.hashCode(), hierarchy);
+  }
 }
diff --git a/underlay/src/main/java/bio/terra/tanagra/api/filter/HierarchyIsRootFilter.java b/underlay/src/main/java/bio/terra/tanagra/api/filter/HierarchyIsRootFilter.java
@@ -3,6 +3,7 @@
 import bio.terra.tanagra.underlay.Underlay;
 import bio.terra.tanagra.underlay.entitymodel.Entity;
 import bio.terra.tanagra.underlay.entitymodel.Hierarchy;
+import java.util.Objects;
 import org.slf4j.LoggerFactory;
 
 public class HierarchyIsRootFilter extends EntityFilter {
@@ -16,4 +17,17 @@ public HierarchyIsRootFilter(Underlay underlay, Entity entity, Hierarchy hierarc
   public Hierarchy getHierarchy() {
     return hierarchy;
   }
+
+  @Override
+  public boolean equals(Object o) {
+    if (!super.equals(o)) {
+      return false;
+    }
+    return hierarchy.equals(((HierarchyIsRootFilter) o).hierarchy);
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(super.hashCode(), hierarchy);
+  }
 }
diff --git a/underlay/src/main/java/bio/terra/tanagra/api/filter/PrimaryWithCriteriaFilter.java b/underlay/src/main/java/bio/terra/tanagra/api/filter/PrimaryWithCriteriaFilter.java
@@ -46,6 +46,27 @@ public PrimaryWithCriteriaFilter(
             : ImmutableMap.copyOf(groupByAttributesPerOccurrenceEntity);
     this.groupByCountOperator = groupByCountOperator;
     this.groupByCountValue = groupByCountValue;
+
+    // one-time checks
+    int numSubFiltersPerOccEntity = getNumSubFiltersPerOccEntity();
+    this.subFiltersPerOccurrenceEntity.forEach(
+        (occurrenceEntity, subFilters) -> {
+          if (subFilters.size() != numSubFiltersPerOccEntity) {
+            throw new InvalidQueryException(
+                "There must be the same number of sub filters for each occurrence entity: "
+                    + occurrenceEntity.getName());
+          }
+        });
+
+    int numGroupByAttributesPerOccEntity = getNumGroupByAttributesPerOccEntity();
+    this.groupByAttributesPerOccurrenceEntity.forEach(
+        (occurrenceEntity, groupByAttributes) -> {
+          if (groupByAttributes.size() != numGroupByAttributesPerOccEntity) {
+            throw new InvalidQueryException(
+                "There must be the same number of group by attributes for each occurrence entity: "
+                    + occurrenceEntity.getName());
+          }
+        });
   }
 
   public CriteriaOccurrence getCriteriaOccurrence() {
@@ -56,6 +77,10 @@ public EntityFilter getCriteriaSubFilter() {
     return criteriaSubFilter;
   }
 
+  public int getNumSubFiltersPerOccEntity() {
+    return getSubFilters(criteriaOccurrence.getOccurrenceEntities().get(0)).size();
+  }
+
   public boolean hasSubFilters(Entity occurrenceEntity) {
     return subFiltersPerOccurrenceEntity.containsKey(occurrenceEntity);
   }
@@ -76,22 +101,12 @@ public ImmutableList<Attribute> getGroupByAttributes(Entity occurrenceEntity) {
         : ImmutableList.of();
   }
 
-  public int getNumGroupByAttributes() {
-    int numGroupByAttributes =
-        getGroupByAttributes(criteriaOccurrence.getOccurrenceEntities().get(0)).size();
-    groupByAttributesPerOccurrenceEntity.forEach(
-        (occurrenceEntity, groupByAttributes) -> {
-          if (groupByAttributes.size() != numGroupByAttributes) {
-            throw new InvalidQueryException(
-                "There must be the same number of group by attributes for each occurrence entity: "
-                    + occurrenceEntity.getName());
-          }
-        });
-    return numGroupByAttributes;
+  public int getNumGroupByAttributesPerOccEntity() {
+    return getGroupByAttributes(criteriaOccurrence.getOccurrenceEntities().get(0)).size();
   }
 
   public boolean hasGroupByAttributes() {
-    return getNumGroupByAttributes() > 0;
+    return getNumGroupByAttributesPerOccEntity() > 0;
   }
 
   @Nullable

diff --git a/underlay/src/main/java/bio/terra/tanagra/query/bigquery/translator/BQApiTranslator.java b/underlay/src/main/java/bio/terra/tanagra/query/bigquery/translator/BQApiTranslator.java
@@ -8,6 +8,7 @@
 import bio.terra.tanagra.api.field.HierarchyPathField;
 import bio.terra.tanagra.api.field.RelatedEntityIdCountField;
 import bio.terra.tanagra.api.filter.AttributeFilter;
+import bio.terra.tanagra.api.filter.BooleanAndOrFilter.LogicalOperator;
 import bio.terra.tanagra.api.filter.HierarchyHasAncestorFilter;
 import bio.terra.tanagra.api.filter.HierarchyHasParentFilter;
 import bio.terra.tanagra.api.filter.HierarchyIsLeafFilter;
@@ -46,6 +47,7 @@
 import jakarta.annotation.Nullable;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 
 public final class BQApiTranslator implements ApiTranslator {
   @Override
@@ -151,6 +153,15 @@ public ApiFilterTranslator translator(
     return new BQTemporalPrimaryFilterTranslator(this, temporalPrimaryFilter, attributeSwapFields);
   }
 
+  @Override
+  public Optional<ApiFilterTranslator> mergedTranslatorAttributeFilter(
+      List<AttributeFilter> attributeFilters,
+      LogicalOperator logicalOperator,
+      Map<Attribute, SqlField> attributeSwapFields) {
+    return BQAttributeFilterTranslator.mergedTranslator(
+        this, attributeFilters, logicalOperator, attributeSwapFields);
+  }
+
   @Override
   public String naryFilterOnRepeatedFieldSql(
       SqlField field,

diff --git a/.../java/bio/terra/tanagra/query/bigquery/translator/filter/BQAttributeFilterTranslator.java b/.../java/bio/terra/tanagra/query/bigquery/translator/filter/BQAttributeFilterTranslator.java
@@ -14,7 +14,6 @@
 import bio.terra.tanagra.underlay.entitymodel.Entity;
 import bio.terra.tanagra.underlay.indextable.ITEntityMain;
 import bio.terra.tanagra.underlay.indextable.ITEntitySearchByAttributes;
-import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -162,26 +161,12 @@ public static Optional<ApiFilterTranslator> mergedTranslator(
       List<AttributeFilter> attributeFilters,
       LogicalOperator logicalOperator,
       Map<Attribute, SqlField> attributeSwapFields) {
-    if (attributeFilters.isEmpty()) {
-      return Optional.empty();
-    }
-
     Entity firstEntity = attributeFilters.get(0).getEntity();
-    String firstEntityName = firstEntity.getName();
-    List<String> allFilterAttributeNames = new ArrayList<>();
-
-    // condition-1: all filters must be on the same entity
-    if (attributeFilters.stream()
-        .anyMatch(
-            filter -> {
-              allFilterAttributeNames.addAll(filter.getFilterAttributeNames());
-              return !filter.getEntity().getName().equals(firstEntityName);
-            })) {
-      return Optional.empty();
-    }
+    List<String> allFilterAttributeNames =
+        attributeFilters.stream().flatMap(l -> l.getFilterAttributeNames().stream()).toList();
 
-    // condition-2: all attrs must be optimized for search together
-    // if only (1), the filters are already run on the same table, no change needed
+    // All attrs must be optimized for search together.
+    // Otherwise, the filters may already run on the same table, no change needed
     return firstEntity.containsOptimizeSearchByAttributes(allFilterAttributeNames)
         ? Optional.of(
             new BQAttributeFilterTranslator(

diff --git a/...terra/tanagra/query/bigquery/translator/filter/BQPrimaryWithCriteriaFilterTranslator.java b/...terra/tanagra/query/bigquery/translator/filter/BQPrimaryWithCriteriaFilterTranslator.java
@@ -196,7 +196,7 @@ public String buildSql(SqlParams sqlParams, String tableAlias) {
     }
 
     List<String> groupByFieldsSql = new ArrayList<>();
-    for (int i = 0; i < primaryWithCriteriaFilter.getNumGroupByAttributes(); i++) {
+    for (int i = 0; i < primaryWithCriteriaFilter.getNumGroupByAttributesPerOccEntity(); i++) {
       groupByFieldsSql.add(groupByFieldAliasPrefix + i);
     }
     String groupByFieldsSqlJoined = String.join(", ", groupByFieldsSql);

diff --git a/underlay/src/main/java/bio/terra/tanagra/query/sql/translator/ApiTranslator.java b/underlay/src/main/java/bio/terra/tanagra/query/sql/translator/ApiTranslator.java
@@ -32,7 +32,6 @@
 import bio.terra.tanagra.api.shared.UnaryOperator;
 import bio.terra.tanagra.exception.InvalidQueryException;
 import bio.terra.tanagra.exception.SystemException;
-import bio.terra.tanagra.query.bigquery.translator.filter.BQAttributeFilterTranslator;
 import bio.terra.tanagra.query.sql.SqlField;
 import bio.terra.tanagra.query.sql.SqlParams;
 import bio.terra.tanagra.query.sql.SqlQueryField;
@@ -407,17 +406,38 @@ default ApiFilterTranslator translator(
     } else if (entityFilter instanceof TemporalPrimaryFilter) {
       return translator((TemporalPrimaryFilter) entityFilter, attributeSwapFields);
     } else {
-      throw new InvalidQueryException("No SQL translator defined for filter");
+      throw new InvalidQueryException(
+          "No SQL translator defined for filter type: " + entityFilter.getClass().getSimpleName());
     }
   }
 
-  default <T extends EntityFilter> Optional<ApiFilterTranslator> mergedTranslator(
-      List<T> entityFilters,
+  Optional<ApiFilterTranslator> mergedTranslatorAttributeFilter(
+      List<AttributeFilter> attributeFilters,
+      LogicalOperator logicalOperator,
+      Map<Attribute, SqlField> attributeSwapFields);
+
+  default Optional<ApiFilterTranslator> mergedTranslator(
+      List<EntityFilter> entityFilters,
       LogicalOperator logicalOperator,
       Map<Attribute, SqlField> attributeSwapFields) {
-    if (entityFilters.get(0) instanceof AttributeFilter) {
-      return BQAttributeFilterTranslator.mergedTranslator(
-          this,
+    if (entityFilters.isEmpty()) {
+      return Optional.empty();
+    }
+
+    // 1) Common to ALL filter types: Must be of the same type & on same entity
+    if (!EntityFilter.areSameFilterTypeAndEntity(entityFilters)) {
+      return Optional.empty();
+    }
+
+    // 2) Filter-type specific: Check the filter fields that lead to sub-queries or nested-filters.
+    // Example (2a): Attribute, Test filters are terminal since they do not involve sub-filters
+    // during translation. Hence, it is sufficient to check attribute field for mergeability.
+    // Example (2b): BooleanNot, BooleanAndOr, PrimaryWithCriteria filter are NOT terminal since
+    // they always include sub filters during translation. Hence, it is necessary to check all
+    // other fields that lead to a sub-filter.
+    EntityFilter entityFilter = entityFilters.get(0);
+    if (entityFilter instanceof AttributeFilter) {
+      return mergedTranslatorAttributeFilter(
           entityFilters.stream().map(f -> (AttributeFilter) f).toList(),
           logicalOperator,
           attributeSwapFields);

diff --git a/...main/java/bio/terra/tanagra/query/sql/translator/filter/BooleanAndOrFilterTranslator.java b/...main/java/bio/terra/tanagra/query/sql/translator/filter/BooleanAndOrFilterTranslator.java
@@ -1,7 +1,6 @@
 package bio.terra.tanagra.query.sql.translator.filter;
 
 import bio.terra.tanagra.api.filter.BooleanAndOrFilter;
-import bio.terra.tanagra.api.filter.EntityFilter;
 import bio.terra.tanagra.query.sql.SqlField;
 import bio.terra.tanagra.query.sql.SqlParams;
 import bio.terra.tanagra.query.sql.translator.ApiFilterTranslator;
@@ -22,16 +21,11 @@ public BooleanAndOrFilterTranslator(
     super(apiTranslator, attributeSwapFields);
     this.booleanAndOrFilter = booleanAndOrFilter;
 
-    // Get a single merged translator if the sub-filters are mergeable: must be of the same type
-    // Additional checks may be needed for individual sub-filter types
     Optional<ApiFilterTranslator> mergedTranslator =
-        EntityFilter.areSameFilterType(booleanAndOrFilter.getSubFilters())
-            ? apiTranslator.mergedTranslator(
-                booleanAndOrFilter.getSubFilters(),
-                booleanAndOrFilter.getOperator(),
-                attributeSwapFields)
-            : Optional.empty();
-
+        apiTranslator.mergedTranslator(
+            booleanAndOrFilter.getSubFilters(),
+            booleanAndOrFilter.getOperator(),
+            attributeSwapFields);
     this.subFilterTranslators =
         mergedTranslator
             .map(List::of)