FrankChen021 commented on code in PR #20469:
URL: https://github.com/apache/druid/pull/20469#discussion_r4155322849
##########
processing/src/main/java/org/apache/druid/query/dimension/ListFilteredDimensionSpec.java:
##########
@@ -122,9 +124,9 @@ public static IdMapping buildDenyListIdMapping(
return builder.build();
}
- public static DimensionSelector filterAllowList(Set<String> values,
DimensionSelector selector, boolean forcePredicateFilter)
+ public static DimensionSelector filterAllowList(Set<String> values,
DimensionSelector selector)
Review Comment:
[P1] Preserve the removed public overloads
**Finding:** This change deletes the public three-argument
filterAllowList(...) and filterDenyList(...) overloads from the core processing
API. Existing extensions or downstream code that calls either overload will no
longer compile, and already-linked binaries can fail with NoSuchMethodError
after upgrading; migrating the internal call sites does not preserve that API.
**Suggestion:** Keep deprecated three-argument overloads with their existing
forcePredicateFilter behavior, and route new internal callers through the
two-argument overload.
##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/MultiValueStringOperatorConversions.java:
##########
@@ -375,48 +380,139 @@ public DruidExpression toDruidExpression(
};
Expr expr =
plannerContext.parseExpression(druidExpressions.get(1).getExpression());
- if (druidExpressions.get(0).isSimpleExtraction() && expr.isLiteral()) {
+ if (expr.isLiteral()) {
Object[] lit = expr.eval(InputBindings.nilBindings()).asArray();
if (lit == null || lit.length == 0) {
- return null;
+ return druidExpressions.get(0).isSimpleExtraction()
+ ? null
+ : DruidExpression.ofExpression(ColumnType.STRING, builder,
druidExpressions);
}
HashSet<String> literals = Sets.newHashSetWithExpectedSize(lit.length);
for (Object o : lit) {
literals.add(Evals.asString(o));
}
- DruidExpression druidExpression = DruidExpression.ofVirtualColumn(
- Calcites.getColumnTypeForRelDataType(rexNode.getType()),
- builder,
+ final DruidExpression druidExpression =
toFilteredVirtualColumnExpression(
+ plannerContext,
+ rowSignature,
+ rexNode,
druidExpressions,
- (name, outputType, parser, self) -> new ListFilteredVirtualColumn(
- name,
- self.getArguments().get(0)
- .getSimpleExtraction()
-
.toDimensionSpec(druidExpressions.get(0).getDirectColumn(), outputType),
- literals,
- isAllowList()
- )
+ builder,
+ (name, delegate) -> new ListFilteredVirtualColumn(name, delegate,
literals, isAllowList())
);
-
- // if the join expression VC registry is present, it means that this
expression is part of a join condition
- // and since that's the case, create virtual column here itself for
optimized usage in join matching
- if (plannerContext.getJoinExpressionVirtualColumnRegistry() != null) {
- String virtualColumnName =
plannerContext.getJoinExpressionVirtualColumnRegistry()
-
.getOrCreateVirtualColumnForExpression(
- druidExpression,
- ColumnType.STRING
- );
- return DruidExpression.ofColumn(ColumnType.STRING,
virtualColumnName);
+ if (druidExpression != null) {
+ return druidExpression;
}
-
- return druidExpression;
}
return DruidExpression.ofExpression(ColumnType.STRING, builder,
druidExpressions);
}
}
+ /**
+ * Creates a {@link VirtualColumn} for a multi-value string filtering
function given the {@link DimensionSpec} of the
+ * multi-value string to filter
+ */
+ @FunctionalInterface
+ private interface FilteredVirtualColumnCreator
+ {
+ VirtualColumn create(String name, DimensionSpec delegate);
+ }
+
+ /**
+ * Builds a {@link DruidExpression.NodeType#SPECIALIZED} expression for a
multi-value string filtering function whose
+ * first argument is the multi-value string to filter, if the first argument
can be used as the delegate
+ * {@link DimensionSpec} of a specialized filtering virtual column. Returns
null if it cannot, in which case callers
+ * should fall back to a plain expression.
+ *
+ * @see #toFilterDelegate(PlannerContext, RowSignature, DruidExpression)
+ */
+ @Nullable
+ private static DruidExpression toFilteredVirtualColumnExpression(
+ PlannerContext plannerContext,
+ RowSignature rowSignature,
+ RexNode rexNode,
+ List<DruidExpression> druidExpressions,
+ DruidExpression.ExpressionGenerator builder,
+ FilteredVirtualColumnCreator virtualColumnCreator
+ )
+ {
+ final DruidExpression delegate = toFilterDelegate(plannerContext,
rowSignature, druidExpressions.get(0));
+ if (delegate == null) {
+ return null;
+ }
+ final List<DruidExpression> arguments = new ArrayList<>(druidExpressions);
+ arguments.set(0, delegate);
+
+ final DruidExpression druidExpression = DruidExpression.ofVirtualColumn(
+ Calcites.getColumnTypeForRelDataType(rexNode.getType()),
+ builder,
+ arguments,
+ (name, outputType, parser, self) -> {
+ final DruidExpression arg = self.getArguments().get(0);
+ if (!arg.isSimpleExtraction()) {
+ // the delegate expression was not lifted into its own virtual
column by specialization, so we cannot
+ // reference it by name, fall back to a plain expression virtual
column
+ return self.toExpressionVirtualColumn(name, outputType, parser);
+ }
+ return virtualColumnCreator.create(
+ name,
+ arg.getSimpleExtraction().toDimensionSpec(arg.getDirectColumn(),
outputType)
+ );
+ }
+ );
+
+ // if the join expression VC registry is present, it means that this
expression is part of a join condition
+ // and since that's the case, create virtual column here itself for
optimized usage in join matching
+ if (plannerContext.getJoinExpressionVirtualColumnRegistry() != null) {
+ String virtualColumnName =
plannerContext.getJoinExpressionVirtualColumnRegistry()
+
.getOrCreateVirtualColumnForExpression(
+ druidExpression,
+ ColumnType.STRING
+ );
+ return DruidExpression.ofColumn(ColumnType.STRING, virtualColumnName);
+ }
+
+ return druidExpression;
+ }
+
+ /**
+ * Returns a {@link DruidExpression} suitable to use as the delegate {@link
DimensionSpec} of a specialized multi-value
+ * string filtering virtual column, or null if there is no such expression.
+ *
+ * Simple extractions are used directly. Expressions of a single {@link
ColumnType#STRING} input are marked
+ * {@link DruidExpression#asSpecialized()} so that they are lifted into
their own {@link ExpressionVirtualColumn}
+ * which the filtering virtual column can use as its delegate. When the
input column is dictionary encoded, the
+ * expression virtual column produces a dimension selector backed by the
input column dictionary which defers
+ * expression evaluation until values are looked up, allowing the filtering
virtual column to evaluate the expression
+ * once per dictionary value instead of for every row.
+ */
+ @Nullable
+ private static DruidExpression toFilterDelegate(
+ PlannerContext plannerContext,
+ RowSignature rowSignature,
+ DruidExpression arg
+ )
+ {
+ if (arg.isSimpleExtraction()) {
+ return arg;
+ }
+ if (arg.getType() == DruidExpression.NodeType.SPECIALIZED ||
!ColumnType.STRING.equals(arg.getDruidType())) {
+ return null;
+ }
+ final Expr.BindingAnalysis bindingAnalysis =
plannerContext.parseExpression(arg.getExpression()).analyzeInputs();
+ if (bindingAnalysis.getRequiredBindings().size() != 1
Review Comment:
[P2] Do not defer nondeterministic delegates by dictionary
**Finding:** The new eligibility check accepts any one-binding scalar string
expression without checking BindingAnalysis.isNonDeterministic(). A
string-producing one-column expression that includes NOW() can therefore be
lifted into an ExpressionVirtualColumn; its single-input plan uses
SingleStringInputDeferredEvaluationExpressionDimensionSelector and evaluates
the expression once per dictionary value instead of per row, changing filtered
or grouped results for nondeterministic expressions.
**Suggestion:** Reject nondeterministic binding analyses here (or force the
plain expression path) and add a regression test covering a one-column
nondeterministic delegate.
--
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]
