From 096e842fe8d3a10eb98cc0169dd72192b483598a Mon Sep 17 00:00:00 2001 From: Dexter Amundsen Date: Mon, 3 Feb 2025 15:04:47 -0800 Subject: [PATCH] update checks in BQAttributeFilterTranslator --- .../tanagra/api/filter/EntityFilter.java | 11 ++++- .../api/filter/HierarchyHasParentFilter.java | 15 +++++++ .../api/filter/HierarchyIsLeafFilter.java | 14 +++++++ .../api/filter/HierarchyIsMemberFilter.java | 14 +++++++ .../api/filter/HierarchyIsRootFilter.java | 14 +++++++ .../api/filter/PrimaryWithCriteriaFilter.java | 41 +++++++++++++------ .../bigquery/translator/BQApiTranslator.java | 11 +++++ .../filter/BQAttributeFilterTranslator.java | 23 ++--------- ...BQPrimaryWithCriteriaFilterTranslator.java | 2 +- .../query/sql/translator/ApiTranslator.java | 34 +++++++++++---- .../filter/BooleanAndOrFilterTranslator.java | 14 ++----- 11 files changed, 141 insertions(+), 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 index 0e6275535..a6ceab00f 100644 --- 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 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 filters) { + public static boolean areSameFilterTypeAndEntity(List 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 index a920601b7..90d7417de 100644 --- 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 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 index e45dd5756..7abc777f3 100644 --- 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 index a6fc6c27a..feb82cf55 100644 --- 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 index b3f0573bf..34fcb304e 100644 --- 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 index c15e0878e..50624f4fb 100644 --- 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 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 index 40b14e66c..937b530cd 100644 --- 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 mergedTranslatorAttributeFilter( + List attributeFilters, + LogicalOperator logicalOperator, + Map attributeSwapFields) { + return BQAttributeFilterTranslator.mergedTranslator( + this, attributeFilters, logicalOperator, attributeSwapFields); + } + @Override public String naryFilterOnRepeatedFieldSql( SqlField field, diff --git a/underlay/src/main/java/bio/terra/tanagra/query/bigquery/translator/filter/BQAttributeFilterTranslator.java b/underlay/src/main/java/bio/terra/tanagra/query/bigquery/translator/filter/BQAttributeFilterTranslator.java index f69c9d248..23530af2e 100644 --- a/underlay/src/main/java/bio/terra/tanagra/query/bigquery/translator/filter/BQAttributeFilterTranslator.java +++ b/underlay/src/main/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 mergedTranslator( List attributeFilters, LogicalOperator logicalOperator, Map attributeSwapFields) { - if (attributeFilters.isEmpty()) { - return Optional.empty(); - } - Entity firstEntity = attributeFilters.get(0).getEntity(); - String firstEntityName = firstEntity.getName(); - List 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 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/underlay/src/main/java/bio/terra/tanagra/query/bigquery/translator/filter/BQPrimaryWithCriteriaFilterTranslator.java b/underlay/src/main/java/bio/terra/tanagra/query/bigquery/translator/filter/BQPrimaryWithCriteriaFilterTranslator.java index ccaf7c7cb..0b351f260 100644 --- a/underlay/src/main/java/bio/terra/tanagra/query/bigquery/translator/filter/BQPrimaryWithCriteriaFilterTranslator.java +++ b/underlay/src/main/java/bio/terra/tanagra/query/bigquery/translator/filter/BQPrimaryWithCriteriaFilterTranslator.java @@ -196,7 +196,7 @@ public String buildSql(SqlParams sqlParams, String tableAlias) { } List 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 index 1a6315a47..ecfaf625b 100644 --- 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 Optional mergedTranslator( - List entityFilters, + Optional mergedTranslatorAttributeFilter( + List attributeFilters, + LogicalOperator logicalOperator, + Map attributeSwapFields); + + default Optional mergedTranslator( + List entityFilters, LogicalOperator logicalOperator, Map 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/underlay/src/main/java/bio/terra/tanagra/query/sql/translator/filter/BooleanAndOrFilterTranslator.java b/underlay/src/main/java/bio/terra/tanagra/query/sql/translator/filter/BooleanAndOrFilterTranslator.java index 3958cdbed..cebbf3982 100644 --- a/underlay/src/main/java/bio/terra/tanagra/query/sql/translator/filter/BooleanAndOrFilterTranslator.java +++ b/underlay/src/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 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)