FrankChen021 commented on code in PR #20469:
URL: https://github.com/apache/druid/pull/20469#discussion_r4306046527
##########
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:
I rechecked head 73062cd and am reconfirming this finding. The guard at line
510 still omits `BindingAnalysis.isNonDeterministic()`. An expression such as
`if(dim3 == null, null, now())` can pass the nil-result check, then be
evaluated once while building the IdMapping and again by lookupName, allowing
the same dictionary ID to be classified inconsistently. Please reject
nondeterministic analyses before specialization and add a regression. Reviewed
9 of 9 changed files.
<!-- mergelens:review -->
--
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]
