Guard against exponential increase of filters during CNF conversion #12314
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package org.apache.druid.segment.filter.cnf; | ||
|
|
||
| import org.apache.druid.java.util.common.StringUtils; | ||
|
|
||
| public class CNFFilterExplosionException extends Exception | ||
|
There was a problem hiding this comment. should this be a There was a problem hiding this comment. Yes, There was a problem hiding this comment. i think that makes sense, looking closer I see now that these exceptions are always eaten and don't make it to the user, i admittedly just did a drive by scan of this PR earlier and thought they might make it to the surface 😅 though thinking about it, maybe it is worth a log of some sort to know that this is happening so that there is some indicator of why the expected query isn't happening? There was a problem hiding this comment. i guess a log might be kind of noisy if this was coming from the SQL planner, and would need to be collected and printed at the end instead. We have a mechanism for collecting errors this way, but not for informative messages; this could be done at a later time I think (if is actually useful to have a way to know why the planner did or didn't do a think that was asked for). There was a problem hiding this comment. Yes, I agree with that - it would make more sense to have something in a per query structure which is shown to the user. |
||
| { | ||
| public CNFFilterExplosionException(String formatText, Object... arguments) | ||
| { | ||
| super(StringUtils.nonStrictFormat(formatText, arguments)); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just put a log that we are not converting to cnf due to 'CNFFilterExplosionExceptions` so that people debugging the query know whats happening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand where your coming from. In such a case, I would then tend towards not throwing an exception and refactoring the
Filter.toCnf(filter)to return an appropriate state which helps us decide if the conversionWasSucessfull or not. Maybe useOptional.of()so that we avoid throwing exceptions due to performance overheads of exceptions.We can take this up as future work. Maybe when we are making this limit configurable.