jihoonson commented on a change in pull request #12195:
URL: https://github.com/apache/druid/pull/12195#discussion_r791179602
##########
File path:
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerConfig.java
##########
@@ -190,6 +201,24 @@ public PlannerConfig withOverrides(final Map<String,
Object> context)
return newConfig;
}
+ private static int getContextInt(
+ final Map<String, Object> context,
+ final String parameter,
+ final int defaultValue
+ )
+ {
+ final Object value = context.get(parameter);
+ if (value == null) {
+ return defaultValue;
+ } else if (value instanceof String) {
+ return Integer.parseInt((String) value);
+ } else if (value instanceof Integer) {
+ return (Integer) value;
+ } else {
+ throw new IAE("Expected parameter[%s] to be integer", parameter);
+ }
Review comment:
nit: you can use `Numbers.parseInt()` instead.
##########
File path:
sql/src/main/java/org/apache/druid/sql/calcite/run/NativeQueryMaker.java
##########
@@ -122,6 +125,24 @@ public boolean feature(QueryFeature feature)
);
}
}
+ int numFilters =
plannerContext.getPlannerConfig().getMaxNumericInFilters();
+ if (numFilters < 1) {
+ throw new CannotBuildQueryException("maxNumericInFilters must be greater
than 0");
+ }
+ //special corner case handling for numeric IN filters
Review comment:
Can you add some comments on why this edge case is worth an extra check
here and why the scope of the check is specialized to a particular filter shape?
##########
File path:
sql/src/main/java/org/apache/druid/sql/calcite/run/NativeQueryMaker.java
##########
@@ -122,6 +125,24 @@ public boolean feature(QueryFeature feature)
);
}
}
+ int numFilters =
plannerContext.getPlannerConfig().getMaxNumericInFilters();
+ if (numFilters < 1) {
+ throw new CannotBuildQueryException("maxNumericInFilters must be greater
than 0");
Review comment:
@somu-imply `CannotBuildQueryException` doesn't seem right because it's
a configuration error, not query planning error.
> Is it helpful to add a hint using `PlannerContext#setPlanningError` here?
I don't see any usages of that in the `NativeQueryMaker` so I might be wrong.
```
/**
* Sets the planning error in the context that will be shown to the user
if the SQL query cannot be translated
* to a native query. This error is often a hint and thus should be
phrased as such. Also, the final plan can
* be very different from SQL that user has written. So again, the error
should be phrased to indicate this gap
* clearly.
*/
public void setPlanningError(String formatText, Object... arguments)
```
@LakshSingla Here is the javadoc of `setPlanningError`. This method can be
used when there is an exception thrown during query planning that can be useful
for users to fix their queries by themselves to avoid the planning error. Since
`queryMaker` is called after query planning is done, it seems fine to not use
it.
##########
File path:
sql/src/main/java/org/apache/druid/sql/calcite/run/NativeQueryMaker.java
##########
@@ -122,6 +125,24 @@ public boolean feature(QueryFeature feature)
);
}
}
+ int numFilters =
plannerContext.getPlannerConfig().getMaxNumericInFilters();
+ if (numFilters < 1) {
+ throw new CannotBuildQueryException("maxNumericInFilters must be greater
than 0");
+ }
+ //special corner case handling for numeric IN filters
+ if (query.getFilter() instanceof OrDimFilter) {
+ OrDimFilter orDimFilter = (OrDimFilter) query.getFilter();
+ if (orDimFilter.getFields().size() > numFilters) {
+ String dimension = ((BoundDimFilter)
(orDimFilter.getFields().get(0))).getDimension();
+ throw new CannotBuildQueryException(StringUtils.format(
+ "Filters for column [%s] exceeds configured filter limit of [%s].
Cast [%s] values to String",
Review comment:
This error message seems not intuitive as it suggest casting integers to
strings. It could be useful to add more details on why they should consider the
casting.
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -6903,6 +6903,27 @@ public void testMaxSubqueryRows() throws Exception
);
}
+ @Test
+ public void testZeroMaxNumericInFilter() throws Exception
Review comment:
Yes, we should add a test that covers this case. @somu-imply can you
please add one?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
