[GitHub] [druid] suneet-s commented on a change in pull request #10758: Retain order of AND, OR filter children.

[GitBox <gi...@apache.org>]

suneet-s commented on a change in pull request #10758:
URL: https://github.com/apache/druid/pull/10758#discussion_r558804246



##########
File path: processing/src/main/java/org/apache/druid/segment/filter/Filters.java
##########
@@ -480,68 +485,105 @@ public static boolean shouldUseBitmapIndex(
   }
 
   /**
-   * Create a filter representing an AND relationship across a list of filters.
+   * Create a filter representing an AND relationship across a list of filters. Deduplicates filters, flattens stacks,
+   * and removes literal "false" filters.
    *
-   * @param filterList List of filters
+   * @param filters List of filters
    *
-   * @return If filterList has more than one element, return an AND filter composed of the filters from filterList
-   * If filterList has a single element, return that element alone
-   * If filterList is empty, return null
+   * @return If "filters" has more than one filter remaining after processing, returns {@link AndFilter}.
+   * If "filters" has a single element remaining after processing, return that filter alone.
+   *
+   * @throws IllegalArgumentException if "filters" is empty
    */
-  @Nullable
-  public static Filter and(List<Filter> filterList)
+  public static Filter and(final List<Filter> filters)
   {
-    if (filterList.isEmpty()) {
-      return null;
-    }
+    return maybeAnd(filters).orElseThrow(() -> new IAE("Expected nonempty filters list"));
+  }
 
-    if (filterList.size() == 1) {
-      return filterList.get(0);
+  /**
+   * Like {@link #and}, but returns an empty Optional instead of throwing an exception if "filters" is empty.
+   */
+  public static Optional<Filter> maybeAnd(List<Filter> filters)
+  {
+    if (filters.isEmpty()) {
+      return Optional.empty();
     }
 
-    return new AndFilter(filterList);
+    final List<Filter> filtersToUse = flattenAndChildren(filters);
+
+    if (filtersToUse.isEmpty()) {
+      assert !filters.isEmpty();
+      // Original "filters" list must have been 100% literally-true filters.
+      return Optional.of(TrueFilter.instance());
+    } else if (filtersToUse.stream().anyMatch(filter -> filter instanceof FalseFilter)) {

Review comment:
       nit: You can avoid this `anyMatch` check by doing the FalseFilter optimization in `flattenAndChildren` by making it return a single `FalseFilter`




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org