FrankChen021 commented on code in PR #20469:
URL: https://github.com/apache/druid/pull/20469#discussion_r4302675525
##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/MultiValueStringOperatorConversions.java:
##########
@@ -375,48 +380,152 @@ 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;
+ }
+ // virtual columns created for join conditions are not always carried into
the native query when joins are nested,
+ // so leave expressions in join conditions inline
+ if (plannerContext.getJoinExpressionVirtualColumnRegistry() != null) {
+ return null;
+ }
+ if (arg.getType() == DruidExpression.NodeType.SPECIALIZED ||
!ColumnType.STRING.equals(arg.getDruidType())) {
+ return null;
+ }
+ final Expr expr = plannerContext.parseExpression(arg.getExpression());
+ final Expr.BindingAnalysis bindingAnalysis = expr.analyzeInputs();
+ if (bindingAnalysis.getRequiredBindings().size() != 1
Review Comment:
[P2] Reject nondeterministic deferred delegates
**Finding:** The new eligibility test does not check
`bindingAnalysis.isNonDeterministic()`. A one-input string expression
containing a nondeterministic subexpression can therefore be lifted into the
deferred dictionary-backed selector, where filtering mappings assume a stable
value per dictionary ID even though evaluations can change between mapping
construction and row or name lookup, producing incorrect results.
**Suggestion:** Return null for nondeterministic binding analyses so those
expressions remain on the row-wise expression path, and add a regression
covering a one-input string expression containing `now()`.
##########
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 removed public selector helper overloads
**Finding:** This change deletes the public static
`filterAllowList(Set<String>, DimensionSelector, boolean)` and
`filterDenyList(Set<String>, DimensionSelector, boolean)` method descriptors.
Existing extensions or downstream code using either overload will fail to
compile or throw `NoSuchMethodError` after upgrading, since the new
two-argument methods do not preserve source or binary compatibility.
**Suggestion:** Retain deprecated three-argument forwarding overloads while
routing them through the duplicate-preserving implementation.
--
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]
