Skip to content

[BugFix] Fix relevance query function over optimization issue in ReduceExpressionsRule #3851

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
qianheng-aws merged 4 commits into from
Jul 9, 2025
Merged

[BugFix] Fix relevance query function over optimization issue in ReduceExpressionsRule #3851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8a367f2
Fix RelevanceQueryFunction over optimization issue by ReduceExpressio…
songkant-aws Jul 4, 2025
44bdd13
Fix PredicateAnalyzerTest
songkant-aws Jul 6, 2025
9d4c890
Address comments
songkant-aws Jul 8, 2025
348ce41
Fix spotless check
songkant-aws Jul 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,6 +676,13 @@ public RexNode visitRelevanceFieldList(RelevanceFieldList node, CalcitePlanConte
@Override
public RexNode visitUnresolvedArgument(UnresolvedArgument node, CalcitePlanContext context) {
RexNode value = analyze(node.getValue(), context);
return context.relBuilder.alias(value, node.getArgName());
/*
* Calcite SqlStdOperatorTable.AS doesn't have implementor registration in RexImpTable.
* To not block ReduceExpressionsRule constants reduction optimization, use MAP_VALUE_CONSTRUCTOR instead to achieve the same effect.
*/
return context.rexBuilder.makeCall(
SqlStdOperatorTable.MAP_VALUE_CONSTRUCTOR,
context.rexBuilder.makeLiteral(node.getArgName()),
value);
}
}
17 changes: 17 additions & 0 deletions core/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.apache.calcite.rel.RelNode;
import org.apache.calcite.rel.RelShuttle;
import org.apache.calcite.rel.core.TableScan;
import org.apache.calcite.rex.RexCall;
import org.apache.calcite.rex.RexInputRef;
import org.apache.calcite.rex.RexNode;
import org.apache.calcite.rex.RexVisitorImpl;
Expand Down Expand Up @@ -330,4 +331,20 @@ public Void visitChildren(Node node, Object context) {
};
node.accept(leafVisitor, null);
}

/**
* Return the first value RexNode of the valid map RexCall structure
*
* @param rexNode RexNode that expects type of MAP_VALUE_CONSTRUCTOR RexCall
* @return first value of the valid map RexCall
*/
static RexNode derefMapCall(RexNode rexNode) {
if (rexNode instanceof RexCall) {
RexCall call = (RexCall) rexNode;
if (call.getOperator() == SqlStdOperatorTable.MAP_VALUE_CONSTRUCTOR) {
return call.getOperands().get(1);
}
}
return rexNode;
}
}
6 changes: 3 additions & 3 deletions core/src/main/java/org/opensearch/sql/expression/function/PPLBuiltinOperators.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,11 +338,11 @@ public class PPLBuiltinOperators extends ReflectiveSqlOperatorTable {
public static final SqlOperator MATCH_PHRASE_PREFIX =
RELEVANCE_QUERY_FUNCTION_INSTANCE.toUDF("match_phrase_prefix");
public static final SqlOperator SIMPLE_QUERY_STRING =
RELEVANCE_QUERY_FUNCTION_INSTANCE.toUDF("simple_query_string");
RELEVANCE_QUERY_FUNCTION_INSTANCE.toUDF("simple_query_string", false);
public static final SqlOperator QUERY_STRING =
RELEVANCE_QUERY_FUNCTION_INSTANCE.toUDF("query_string");
RELEVANCE_QUERY_FUNCTION_INSTANCE.toUDF("query_string", false);
public static final SqlOperator MULTI_MATCH =
RELEVANCE_QUERY_FUNCTION_INSTANCE.toUDF("multi_match");
RELEVANCE_QUERY_FUNCTION_INSTANCE.toUDF("multi_match", false);

/**
* Invoking an implementor registered in {@link RexImpTable}, need to use reflection since they're
Expand Down
23 changes: 14 additions & 9 deletions core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@
import org.opensearch.sql.calcite.udf.udaf.PercentileApproxFunction;
import org.opensearch.sql.calcite.udf.udaf.TakeAggFunction;
import org.opensearch.sql.calcite.utils.OpenSearchTypeFactory;
import org.opensearch.sql.calcite.utils.PlanUtils;
import org.opensearch.sql.calcite.utils.UserDefinedFunctionUtils;
import org.opensearch.sql.exception.ExpressionEvaluationException;
import org.opensearch.sql.executor.QueryType;
Expand Down Expand Up @@ -1033,19 +1034,23 @@ void populate() {

register(
TAKE,
(distinct, field, argList, ctx) ->
TransferUserDefinedAggFunction(
TakeAggFunction.class,
"TAKE",
UserDefinedFunctionUtils.getReturnTypeInferenceForArray(),
List.of(field),
argList,
ctx.relBuilder));
(distinct, field, argList, ctx) -> {
List<RexNode> newArgList =
argList.stream().map(PlanUtils::derefMapCall).collect(Collectors.toList());
return TransferUserDefinedAggFunction(
TakeAggFunction.class,
"TAKE",
UserDefinedFunctionUtils.getReturnTypeInferenceForArray(),
List.of(field),
newArgList,
ctx.relBuilder);
});

register(
PERCENTILE_APPROX,
(distinct, field, argList, ctx) -> {
List<RexNode> newArgList = new ArrayList<>(argList);
List<RexNode> newArgList =
argList.stream().map(PlanUtils::derefMapCall).collect(Collectors.toList());
newArgList.add(ctx.rexBuilder.makeFlag(field.getType().getSqlTypeName()));
return TransferUserDefinedAggFunction(
PercentileApproxFunction.class,
Expand Down
20 changes: 19 additions & 1 deletion core/src/main/java/org/opensearch/sql/expression/function/UserDefinedFunctionBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,19 @@ public interface UserDefinedFunctionBuilder {
UDFOperandMetadata getOperandMetadata();

default SqlUserDefinedFunction toUDF(String functionName) {
return toUDF(functionName, true);
}

/**
* In some rare cases, we need to call out the UDF to be not deterministic to avoid Volcano
* planner over-optimization. For example, we don't need ReduceExpressionsRule to optimize
* relevance query UDF.
*
* @param functionName UDF name to be registered
* @param isDeterministic Specified isDeterministic flag
* @return Calcite SqlUserDefinedFunction
*/
default SqlUserDefinedFunction toUDF(String functionName, boolean isDeterministic) {
Copy link
Member

@LantaoJin LantaoJin Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you combine the methods toUDF in L35? We don't want to keep diff implementations.

Copy link
Contributor Author

@songkant-aws songkant-aws Jul 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

SqlIdentifier udfLtrimIdentifier =
new SqlIdentifier(Collections.singletonList(functionName), null, SqlParserPos.ZERO, null);
return new SqlUserDefinedFunction(
Expand All @@ -41,6 +54,11 @@ default SqlUserDefinedFunction toUDF(String functionName) {
getReturnTypeInference(),
InferTypes.ANY_NULLABLE,
getOperandMetadata(),
getFunction());
getFunction()) {
@Override
public boolean isDeterministic() {
return isDeterministic;
}
};
}
}
76 changes: 38 additions & 38 deletions core/src/main/java/org/opensearch/sql/expression/function/udf/RelevanceQueryFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,49 +41,49 @@ public UDFOperandMetadata getOperandMetadata() {
(CompositeOperandTypeChecker)
OperandTypes.family(
ImmutableList.of(
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING),
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP),
i -> i > 1 && i < 14) // Parameters 3-14 are optional
.or(
OperandTypes.family(
ImmutableList.of(
SqlTypeFamily.MAP,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING,
SqlTypeFamily.STRING),
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP,
SqlTypeFamily.MAP),
i -> i > 1 && i < 25))); // Parameters 3-25 are optional
}

Expand Down
29 changes: 27 additions & 2 deletions integ-test/src/test/java/org/opensearch/sql/ppl/RelevanceFunctionIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,13 +144,38 @@ public void test_mixed_relevance_function_and_normal_filter() throws IOException
assertEquals(8, result2.getInt("total"));
}

@Test
public void test_single_field_relevance_function_and_implicit_timestamp_filter()
throws IOException {
String query1 =
"SOURCE="
+ TEST_INDEX_BEER
+ " | WHERE match('Tags', 'brewing taste', operator='AND') and CreationDate >"
+ " '2014-01-20 00:00:00.000' and CreationDate < '2018-01-20 00:00:00.000' | fields"
+ " Tags";
var result1 = executeQuery(query1);
assertEquals(2, result1.getInt("total"));
}

@Test
public void test_multi_fields_relevance_function_and_implicit_timestamp_filter()
throws IOException {
String query1 =
"SOURCE="
+ TEST_INDEX_BEER
+ " | WHERE query_string(['Tags'], 'brewing AND taste') and CreationDate > '2014-01-20"
+ " 00:00:00.000' and CreationDate < '2018-01-20 00:00:00.000' | fields Tags";
var result1 = executeQuery(query1);
assertEquals(2, result1.getInt("total"));
}

@Test
public void not_pushdown_throws_exception() throws IOException {
String query1 =
"SOURCE="
+ TEST_INDEX_BEER
+ " | STATS count(AcceptedAnswerId) as idCount"
+ " | WHERE simple_query_string(['Tags'], 'taste') and idCount > 200";
+ " | EVAL answerId = AcceptedAnswerId + 1"
Copy link
Member

@LantaoJin LantaoJin Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we change this IT case?

Copy link
Contributor Author

@songkant-aws songkant-aws Jul 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I think the previous test case semantic is ambiguous simple_query_string can't query on target field right after an aggregation. Use eval operator to simulate a filter that can't be pushed down makes a lot sense.

+ " | WHERE simple_query_string(['Tags'], 'taste') and answerId > 200";
assertThrows(Exception.class, () -> executeQuery(query1));
}
}
4 changes: 2 additions & 2 deletions ...g-test/src/test/resources/expectedOutput/calcite/explain_multi_fields_relevance_push.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"calcite": {
"logical": "LogicalProject(account_number=[$0], firstname=[$1], address=[$2], balance=[$3], gender=[$4], city=[$5], employer=[$6], state=[$7], age=[$8], email=[$9], lastname=[$10])\n LogicalFilter(condition=[simple_query_string(AS(MAP('name':VARCHAR, 4.0E0:DOUBLE, 'email':VARCHAR, 1.0E0:DOUBLE), 'fields'), AS('gmail':VARCHAR, 'query'), AS('or':VARCHAR, 'default_operator'), AS('english':VARCHAR, 'analyzer'))])\n CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])\n",
"physical": "CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]], PushDownContext=[[PROJECT->[account_number, firstname, address, balance, gender, city, employer, state, age, email, lastname], FILTER->simple_query_string(AS(MAP('name':VARCHAR, 4.0E0:DOUBLE, 'email':VARCHAR, 1.0E0:DOUBLE), 'fields'), AS('gmail':VARCHAR, 'query'), AS('or':VARCHAR, 'default_operator'), AS('english':VARCHAR, 'analyzer'))], OpenSearchRequestBuilder(sourceBuilder={\"from\":0,\"timeout\":\"1m\",\"query\":{\"simple_query_string\":{\"query\":\"gmail\",\"fields\":[\"name^4.0\",\"email^1.0\"],\"analyzer\":\"english\",\"flags\":-1,\"default_operator\":\"or\",\"analyze_wildcard\":false,\"auto_generate_synonyms_phrase_query\":true,\"fuzzy_prefix_length\":0,\"fuzzy_max_expansions\":50,\"fuzzy_transpositions\":true,\"boost\":1.0}},\"_source\":{\"includes\":[\"account_number\",\"firstname\",\"address\",\"balance\",\"gender\",\"city\",\"employer\",\"state\",\"age\",\"email\",\"lastname\"],\"excludes\":[]},\"sort\":[{\"_doc\":{\"order\":\"asc\"}}]}, requestedTotalSize=2147483647, pageSize=null, startFrom=0)])\n"
"logical": "LogicalProject(account_number=[$0], firstname=[$1], address=[$2], balance=[$3], gender=[$4], city=[$5], employer=[$6], state=[$7], age=[$8], email=[$9], lastname=[$10])\n LogicalFilter(condition=[simple_query_string(MAP('fields', MAP('name':VARCHAR, 4.0E0:DOUBLE, 'email':VARCHAR, 1.0E0:DOUBLE)), MAP('query', 'gmail':VARCHAR), MAP('default_operator', 'or':VARCHAR), MAP('analyzer', 'english':VARCHAR))])\n CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])\n",
"physical": "CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]], PushDownContext=[[PROJECT->[account_number, firstname, address, balance, gender, city, employer, state, age, email, lastname], FILTER->simple_query_string(MAP('fields', MAP('name':VARCHAR, 4.0E0:DOUBLE, 'email':VARCHAR, 1.0E0:DOUBLE)), MAP('query', 'gmail':VARCHAR), MAP('default_operator', 'or':VARCHAR), MAP('analyzer', 'english':VARCHAR))], OpenSearchRequestBuilder(sourceBuilder={\"from\":0,\"timeout\":\"1m\",\"query\":{\"simple_query_string\":{\"query\":\"gmail\",\"fields\":[\"name^4.0\",\"email^1.0\"],\"analyzer\":\"english\",\"flags\":-1,\"default_operator\":\"or\",\"analyze_wildcard\":false,\"auto_generate_synonyms_phrase_query\":true,\"fuzzy_prefix_length\":0,\"fuzzy_max_expansions\":50,\"fuzzy_transpositions\":true,\"boost\":1.0}},\"_source\":{\"includes\":[\"account_number\",\"firstname\",\"address\",\"balance\",\"gender\",\"city\",\"employer\",\"state\",\"age\",\"email\",\"lastname\"],\"excludes\":[]},\"sort\":[{\"_doc\":{\"order\":\"asc\"}}]}, requestedTotalSize=2147483647, pageSize=null, startFrom=0)])\n"
}
}
4 changes: 2 additions & 2 deletions ...g-test/src/test/resources/expectedOutput/calcite/explain_single_field_relevance_push.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"calcite": {
"logical": "LogicalProject(account_number=[$0], firstname=[$1], address=[$2], balance=[$3], gender=[$4], city=[$5], employer=[$6], state=[$7], age=[$8], email=[$9], lastname=[$10])\n LogicalFilter(condition=[match(AS($9, 'field'), AS('*@gmail.com':VARCHAR, 'query'), AS('1.0':VARCHAR, 'boost'))])\n CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])\n",
"physical": "CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]], PushDownContext=[[PROJECT->[account_number, firstname, address, balance, gender, city, employer, state, age, email, lastname], FILTER->match(AS($9, 'field'), AS('*@gmail.com':VARCHAR, 'query'), AS('1.0':VARCHAR, 'boost'))], OpenSearchRequestBuilder(sourceBuilder={\"from\":0,\"timeout\":\"1m\",\"query\":{\"match\":{\"email\":{\"query\":\"*@gmail.com\",\"operator\":\"OR\",\"prefix_length\":0,\"max_expansions\":50,\"fuzzy_transpositions\":true,\"lenient\":false,\"zero_terms_query\":\"NONE\",\"auto_generate_synonyms_phrase_query\":true,\"boost\":1.0}}},\"_source\":{\"includes\":[\"account_number\",\"firstname\",\"address\",\"balance\",\"gender\",\"city\",\"employer\",\"state\",\"age\",\"email\",\"lastname\"],\"excludes\":[]},\"sort\":[{\"_doc\":{\"order\":\"asc\"}}]}, requestedTotalSize=2147483647, pageSize=null, startFrom=0)])\n"
"logical": "LogicalProject(account_number=[$0], firstname=[$1], address=[$2], balance=[$3], gender=[$4], city=[$5], employer=[$6], state=[$7], age=[$8], email=[$9], lastname=[$10])\n LogicalFilter(condition=[match(MAP('field', $9), MAP('query', '*@gmail.com':VARCHAR), MAP('boost', '1.0':VARCHAR))])\n CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])\n",
"physical": "CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]], PushDownContext=[[PROJECT->[account_number, firstname, address, balance, gender, city, employer, state, age, email, lastname], FILTER->match(MAP('field', $9), MAP('query', '*@gmail.com':VARCHAR), MAP('boost', '1.0':VARCHAR))], OpenSearchRequestBuilder(sourceBuilder={\"from\":0,\"timeout\":\"1m\",\"query\":{\"match\":{\"email\":{\"query\":\"*@gmail.com\",\"operator\":\"OR\",\"prefix_length\":0,\"max_expansions\":50,\"fuzzy_transpositions\":true,\"lenient\":false,\"zero_terms_query\":\"NONE\",\"auto_generate_synonyms_phrase_query\":true,\"boost\":1.0}}},\"_source\":{\"includes\":[\"account_number\",\"firstname\",\"address\",\"balance\",\"gender\",\"city\",\"employer\",\"state\",\"age\",\"email\",\"lastname\"],\"excludes\":[]},\"sort\":[{\"_doc\":{\"order\":\"asc\"}}]}, requestedTotalSize=2147483647, pageSize=null, startFrom=0)])\n"
}
}
4 changes: 2 additions & 2 deletions opensearch/src/main/java/org/opensearch/sql/opensearch/request/PredicateAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,8 +401,8 @@ private static class AliasPair {
final RexNode alias;

static AliasPair from(RexNode node, String funcName) {
RexCall as = expectCall(node, SqlStdOperatorTable.AS, funcName);
return new AliasPair(as.getOperands().get(0), as.getOperands().get(1));
RexCall mapCall = expectCall(node, SqlStdOperatorTable.MAP_VALUE_CONSTRUCTOR, funcName);
return new AliasPair(mapCall.getOperands().get(1), mapCall.getOperands().get(0));
}

private AliasPair(RexNode value, RexNode alias) {
Expand Down
Loading
Loading