[GitHub] [druid] cheddar commented on a change in pull request #12314: Guard against exponential increase of filters during CNF conversion

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

cheddar commented on a change in pull request #12314:
URL: https://github.com/apache/druid/pull/12314#discussion_r822357002



##########
File path: processing/src/main/java/org/apache/druid/segment/filter/Filters.java
##########
@@ -433,10 +434,15 @@ public static Filter convertToCNFFromQueryContext(Query query, @Nullable Filter
       return null;
     }
     boolean useCNF = query.getContextBoolean(QueryContexts.USE_FILTER_CNF_KEY, QueryContexts.DEFAULT_USE_FILTER_CNF);
-    return useCNF ? Filters.toCnf(filter) : filter;
+    try {
+      return useCNF ? Filters.toCnf(filter) : filter;
+    }
+    catch (CNFFilterExplosionException cnfFilterExplosionException) {
+      return filter; // cannot convert to CNF, return the filter as is

Review comment:
       For clusters where this happens regularly, that can generate a lot of log spew.  You could argue that they should respond to the log spew by not doing the queries, but it would still be a sudden chunk of log spew.  In general, CNF is a potential optimization done by the planner, we don't log other optimization choices that we make, we just make them.  In this case, it's deeming that converting to CNF is more expensive than just leaving it as is, which is yet another optimization decision.  I don't think it needs a log, or if it does, maybe at debug level.




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

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