diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/CastFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/CastFunctionIT.java index 2d87fe536fc..a46cc89d0bb 100644 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/CastFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/CastFunctionIT.java @@ -5,6 +5,7 @@ package org.opensearch.sql.ppl; +import static org.junit.Assert.assertEquals; import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_DATATYPE_NONNUMERIC; import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_DATATYPE_NUMERIC; import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_DATE_FORMATS; @@ -19,6 +20,7 @@ import static org.opensearch.sql.util.MatcherUtils.verifySchema; import java.io.IOException; +import java.util.List; import java.util.Locale; import org.json.JSONObject; import org.junit.Test; @@ -471,4 +473,34 @@ public void testCastDoubleAsString() throws IOException { verifySchema(actual, schema("s", "string")); verifyDataRows(actual, rows("0.0")); } + + /** + * A redundant date/time cast on the filtered field changes only how the predicate is executed + * (native range query instead of a per-document script), never which rows match. Compare the + * wrapped forms against the bare-field form to keep that guarantee enforced. + */ + @Test + public void testRedundantDateCastOnFilteredFieldDoesNotChangeRows() throws IOException { + String template = + "source=%s | where %s | sort strict_date_optional_time | fields strict_date_optional_time"; + JSONObject bare = + executeQuery( + String.format( + Locale.ROOT, + template, + TEST_INDEX_DATE_FORMATS, + "strict_date_optional_time >= '1984-04-12 09:07:42'")); + + for (String wrapped : + List.of( + "timestamp(strict_date_optional_time) >= timestamp('1984-04-12 09:07:42')", + "cast(strict_date_optional_time as timestamp) >= timestamp('1984-04-12 09:07:42')")) { + JSONObject actual = + executeQuery(String.format(Locale.ROOT, template, TEST_INDEX_DATE_FORMATS, wrapped)); + assertEquals( + "wrapping the filtered field in a redundant date cast must not change the result", + bare.getJSONArray("datarows").toString(), + actual.getJSONArray("datarows").toString()); + } + } } diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/ExplainIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/ExplainIT.java index 62eadd7ef5e..62a70963342 100644 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/ExplainIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/ExplainIT.java @@ -76,6 +76,54 @@ public void testFilterByCompareStringTimestampPushDownExplain() throws IOExcepti + "| where birthdate < '2018-11-09 00:00:00.000000000' ")); } + /** + * Wrapping an already timestamp-typed field in timestamp() is a no-op, so the comparison must + * still push down to a native range query instead of falling back to a per-document script. + */ + @Test + public void testFilterTimestampWrappedFieldPushDownExplain() throws IOException { + String expected = loadExpectedPlan("explain_filter_push_timestamp_wrapped_field.yaml"); + assertYamlEqualsIgnoreId( + expected, + explainQueryYaml( + "source=opensearch-sql_test_index_bank" + + "| where timestamp(birthdate) > cast('2016-12-08 00:00:00' as timestamp) " + + "| where timestamp(birthdate) < cast('2018-11-09 00:00:00' as timestamp) ")); + } + + /** + * `field > timestamp('...')` is the shape clients emit for a time-range bound (for example the + * Grafana OpenSearch data source's PPL time filter). PPL coerces the string argument first, so + * the bound arrives as timestamp(cast_to_timestamp('...')); applying timestamp() to a value that + * is already a timestamp is a no-op, so the comparison must still push down to a native range + * query. + */ + @Test + public void testFilterTimestampWrappedBoundPushDownExplain() throws IOException { + String expected = loadExpectedPlan("explain_filter_push_timestamp_wrapped_bound.yaml"); + assertYamlEqualsIgnoreId( + expected, + explainQueryYaml( + "source=opensearch-sql_test_index_bank" + + "| where birthdate > timestamp('2016-12-08 00:00:00') " + + "| where birthdate < timestamp('2018-11-09 00:00:00') ")); + } + + /** + * last_day() takes a single date argument and returns a date, but it changes the value, so it is + * not a redundant conversion and must not be folded to the bare field -- it stays on the script + * path rather than becoming a range query. + */ + @Test + public void testFilterLastDayOverDateFieldNoPushDownExplain() throws IOException { + String expected = loadExpectedPlan("explain_filter_last_day_no_push.yaml"); + assertYamlEqualsIgnoreId( + expected, + explainQueryYaml( + "source=opensearch-sql_test_index_date_formats | fields yyyy-MM-dd" + + "| where last_day(yyyy-MM-dd) = date('2018-11-30') ")); + } + @Test public void testFilterByCompareStringDatePushDownExplain() throws IOException { String expected = loadExpectedPlan("explain_filter_push_compare_date_string.yaml"); diff --git a/integ-test/src/test/resources/expectedOutput/calcite/explain_filter_last_day_no_push.yaml b/integ-test/src/test/resources/expectedOutput/calcite/explain_filter_last_day_no_push.yaml new file mode 100644 index 00000000000..a0154610943 --- /dev/null +++ b/integ-test/src/test/resources/expectedOutput/calcite/explain_filter_last_day_no_push.yaml @@ -0,0 +1,8 @@ +calcite: + logical: | + LogicalSystemLimit(fetch=[10000], type=[QUERY_SIZE_LIMIT]) + LogicalFilter(condition=[=(LAST_DAY($0), DATE('2018-11-30':VARCHAR))]) + LogicalProject(yyyy-MM-dd=[$83]) + CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_date_formats]]) + physical: | + CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_date_formats]], PushDownContext=[[PROJECT->[yyyy-MM-dd], SCRIPT->=(LAST_DAY($0), '2018-11-30'), LIMIT->10000], OpenSearchRequestBuilder(sourceBuilder={"from":0,"size":10000,"timeout":"1m","query":{"script":{"script":{"source":"{\"langType\":\"calcite\",\"script\":\"rO0ABXQDfHsKICAib3AiOiB7CiAgICAibmFtZSI6ICI9IiwKICAgICJraW5kIjogIkVRVUFMUyIsCiAgICAic3ludGF4IjogIkJJTkFSWSIKICB9LAogICJvcGVyYW5kcyI6IFsKICAgIHsKICAgICAgIm9wIjogewogICAgICAgICJuYW1lIjogIkxBU1RfREFZIiwKICAgICAgICAia2luZCI6ICJPVEhFUl9GVU5DVElPTiIsCiAgICAgICAgInN5bnRheCI6ICJGVU5DVElPTiIKICAgICAgfSwKICAgICAgIm9wZXJhbmRzIjogWwogICAgICAgIHsKICAgICAgICAgICJkeW5hbWljUGFyYW0iOiAwLAogICAgICAgICAgInR5cGUiOiB7CiAgICAgICAgICAgICJ1ZHQiOiAiRVhQUl9EQVRFIiwKICAgICAgICAgICAgInR5cGUiOiAiVkFSQ0hBUiIsCiAgICAgICAgICAgICJudWxsYWJsZSI6IHRydWUsCiAgICAgICAgICAgICJwcmVjaXNpb24iOiAtMQogICAgICAgICAgfQogICAgICAgIH0KICAgICAgXSwKICAgICAgImNsYXNzIjogIm9yZy5vcGVuc2VhcmNoLnNxbC5leHByZXNzaW9uLmZ1bmN0aW9uLlVzZXJEZWZpbmVkRnVuY3Rpb25CdWlsZGVyJDEiLAogICAgICAidHlwZSI6IHsKICAgICAgICAidWR0IjogIkVYUFJfREFURSIsCiAgICAgICAgInR5cGUiOiAiVkFSQ0hBUiIsCiAgICAgICAgIm51bGxhYmxlIjogdHJ1ZSwKICAgICAgICAicHJlY2lzaW9uIjogLTEKICAgICAgfSwKICAgICAgImRldGVybWluaXN0aWMiOiB0cnVlLAogICAgICAiZHluYW1pYyI6IGZhbHNlCiAgICB9LAogICAgewogICAgICAiZHluYW1pY1BhcmFtIjogMSwKICAgICAgInR5cGUiOiB7CiAgICAgICAgInVkdCI6ICJFWFBSX0RBVEUiLAogICAgICAgICJ0eXBlIjogIlZBUkNIQVIiLAogICAgICAgICJudWxsYWJsZSI6IHRydWUsCiAgICAgICAgInByZWNpc2lvbiI6IC0xCiAgICAgIH0KICAgIH0KICBdCn0=\"}","lang":"opensearch_compounded_script","params":{"utcTimestamp": 0,"SOURCES":[0,2],"DIGESTS":["yyyy-MM-dd","2018-11-30"]}},"boost":1.0}},"_source":{"includes":["yyyy-MM-dd"]}}, requestedTotalSize=10000, pageSize=null, startFrom=0)]) diff --git a/integ-test/src/test/resources/expectedOutput/calcite/explain_filter_push_timestamp_wrapped_bound.yaml b/integ-test/src/test/resources/expectedOutput/calcite/explain_filter_push_timestamp_wrapped_bound.yaml new file mode 100644 index 00000000000..eadf4e58389 --- /dev/null +++ b/integ-test/src/test/resources/expectedOutput/calcite/explain_filter_push_timestamp_wrapped_bound.yaml @@ -0,0 +1,9 @@ +calcite: + logical: | + LogicalSystemLimit(fetch=[10000], type=[QUERY_SIZE_LIMIT]) + LogicalProject(account_number=[$0], firstname=[$1], address=[$2], birthdate=[$3], gender=[$4], city=[$5], lastname=[$6], balance=[$7], employer=[$8], state=[$9], age=[$10], email=[$11], male=[$12]) + LogicalFilter(condition=[<($3, TIMESTAMP('2018-11-09 00:00:00':VARCHAR))]) + LogicalFilter(condition=[>($3, TIMESTAMP('2016-12-08 00:00:00':VARCHAR))]) + CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_bank]]) + physical: | + CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_bank]], PushDownContext=[[PROJECT->[account_number, firstname, address, birthdate, gender, city, lastname, balance, employer, state, age, email, male], FILTER->SEARCH($3, Sarg[('2016-12-08 00:00:00':VARCHAR..'2018-11-09 00:00:00':VARCHAR)]:VARCHAR), LIMIT->10000], OpenSearchRequestBuilder(sourceBuilder={"from":0,"size":10000,"timeout":"1m","query":{"range":{"birthdate":{"from":"2016-12-08T00:00:00.000Z","to":"2018-11-09T00:00:00.000Z","include_lower":false,"include_upper":false,"format":"date_time","boost":1.0}}},"_source":{"includes":["account_number","firstname","address","birthdate","gender","city","lastname","balance","employer","state","age","email","male"]}}, requestedTotalSize=10000, pageSize=null, startFrom=0)]) diff --git a/integ-test/src/test/resources/expectedOutput/calcite/explain_filter_push_timestamp_wrapped_field.yaml b/integ-test/src/test/resources/expectedOutput/calcite/explain_filter_push_timestamp_wrapped_field.yaml new file mode 100644 index 00000000000..c1c443ac91b --- /dev/null +++ b/integ-test/src/test/resources/expectedOutput/calcite/explain_filter_push_timestamp_wrapped_field.yaml @@ -0,0 +1,9 @@ +calcite: + logical: | + LogicalSystemLimit(fetch=[10000], type=[QUERY_SIZE_LIMIT]) + LogicalProject(account_number=[$0], firstname=[$1], address=[$2], birthdate=[$3], gender=[$4], city=[$5], lastname=[$6], balance=[$7], employer=[$8], state=[$9], age=[$10], email=[$11], male=[$12]) + LogicalFilter(condition=[<(TIMESTAMP($3), TIMESTAMP('2018-11-09 00:00:00':VARCHAR))]) + LogicalFilter(condition=[>(TIMESTAMP($3), TIMESTAMP('2016-12-08 00:00:00':VARCHAR))]) + CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_bank]]) + physical: | + CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_bank]], PushDownContext=[[PROJECT->[account_number, firstname, address, birthdate, gender, city, lastname, balance, employer, state, age, email, male], FILTER->SEARCH(TIMESTAMP($3), Sarg[('2016-12-08 00:00:00':VARCHAR..'2018-11-09 00:00:00':VARCHAR)]:VARCHAR), LIMIT->10000], OpenSearchRequestBuilder(sourceBuilder={"from":0,"size":10000,"timeout":"1m","query":{"range":{"birthdate":{"from":"2016-12-08T00:00:00.000Z","to":"2018-11-09T00:00:00.000Z","include_lower":false,"include_upper":false,"format":"date_time","boost":1.0}}},"_source":{"includes":["account_number","firstname","address","birthdate","gender","city","lastname","balance","employer","state","age","email","male"]}}, requestedTotalSize=10000, pageSize=null, startFrom=0)]) diff --git a/integ-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_filter_last_day_no_push.yaml b/integ-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_filter_last_day_no_push.yaml new file mode 100644 index 00000000000..46c54883f66 --- /dev/null +++ b/integ-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_filter_last_day_no_push.yaml @@ -0,0 +1,10 @@ +calcite: + logical: | + LogicalSystemLimit(fetch=[10000], type=[QUERY_SIZE_LIMIT]) + LogicalFilter(condition=[=(LAST_DAY($0), DATE('2018-11-30':VARCHAR))]) + LogicalProject(yyyy-MM-dd=[$83]) + CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_date_formats]]) + physical: | + EnumerableLimit(fetch=[10000]) + EnumerableCalc(expr#0..94=[{inputs}], expr#95=[LAST_DAY($t83)], expr#96=['2018-11-30':EXPR_DATE VARCHAR], expr#97=[=($t95, $t96)], yyyy-MM-dd=[$t83], $condition=[$t97]) + CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_date_formats]]) diff --git a/integ-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_filter_push_timestamp_wrapped_bound.yaml b/integ-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_filter_push_timestamp_wrapped_bound.yaml new file mode 100644 index 00000000000..46743cfb392 --- /dev/null +++ b/integ-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_filter_push_timestamp_wrapped_bound.yaml @@ -0,0 +1,11 @@ +calcite: + logical: | + LogicalSystemLimit(fetch=[10000], type=[QUERY_SIZE_LIMIT]) + LogicalProject(account_number=[$0], firstname=[$1], address=[$2], birthdate=[$3], gender=[$4], city=[$5], lastname=[$6], balance=[$7], employer=[$8], state=[$9], age=[$10], email=[$11], male=[$12]) + LogicalFilter(condition=[<($3, TIMESTAMP('2018-11-09 00:00:00':VARCHAR))]) + LogicalFilter(condition=[>($3, TIMESTAMP('2016-12-08 00:00:00':VARCHAR))]) + CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_bank]]) + physical: | + EnumerableLimit(fetch=[10000]) + EnumerableCalc(expr#0..18=[{inputs}], expr#19=[Sarg[('2016-12-08 00:00:00':VARCHAR..'2018-11-09 00:00:00':VARCHAR)]:VARCHAR], expr#20=[SEARCH($t3, $t19)], proj#0..12=[{exprs}], $condition=[$t20]) + CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_bank]]) diff --git a/integ-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_filter_push_timestamp_wrapped_field.yaml b/integ-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_filter_push_timestamp_wrapped_field.yaml new file mode 100644 index 00000000000..ecaf2c06282 --- /dev/null +++ b/integ-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_filter_push_timestamp_wrapped_field.yaml @@ -0,0 +1,11 @@ +calcite: + logical: | + LogicalSystemLimit(fetch=[10000], type=[QUERY_SIZE_LIMIT]) + LogicalProject(account_number=[$0], firstname=[$1], address=[$2], birthdate=[$3], gender=[$4], city=[$5], lastname=[$6], balance=[$7], employer=[$8], state=[$9], age=[$10], email=[$11], male=[$12]) + LogicalFilter(condition=[<(TIMESTAMP($3), TIMESTAMP('2018-11-09 00:00:00':VARCHAR))]) + LogicalFilter(condition=[>(TIMESTAMP($3), TIMESTAMP('2016-12-08 00:00:00':VARCHAR))]) + CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_bank]]) + physical: | + EnumerableLimit(fetch=[10000]) + EnumerableCalc(expr#0..18=[{inputs}], expr#19=[TIMESTAMP($t3)], expr#20=[Sarg[('2016-12-08 00:00:00':VARCHAR..'2018-11-09 00:00:00':VARCHAR)]:VARCHAR], expr#21=[SEARCH($t19, $t20)], proj#0..12=[{exprs}], $condition=[$t21]) + CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_bank]]) diff --git a/integ-test/src/test/resources/expectedOutput/ppl/explain_filter_last_day_no_push.yaml b/integ-test/src/test/resources/expectedOutput/ppl/explain_filter_last_day_no_push.yaml new file mode 100644 index 00000000000..a04d52a3199 --- /dev/null +++ b/integ-test/src/test/resources/expectedOutput/ppl/explain_filter_last_day_no_push.yaml @@ -0,0 +1,20 @@ +root: + name: ProjectOperator + description: + fields: "[yyyy-MM-dd]" + children: + - name: FilterOperator + description: + conditions: "=(last_day(yyyy-MM-dd), date(\"2018-11-30\"))" + children: + - name: ProjectOperator + description: + fields: "[yyyy-MM-dd]" + children: + - name: OpenSearchIndexScan + description: + request: "OpenSearchQueryRequest(indexName=opensearch-sql_test_index_date_formats,\ + \ sourceBuilder={\"from\":0,\"size\":10000,\"timeout\":\"1m\",\"\ + _source\":{\"includes\":[\"yyyy-MM-dd\"]}}, pitId=*,\ + \ cursorKeepAlive=1m, searchAfter=null, searchResponse=null)" + children: [] diff --git a/integ-test/src/test/resources/expectedOutput/ppl/explain_filter_push_timestamp_wrapped_bound.yaml b/integ-test/src/test/resources/expectedOutput/ppl/explain_filter_push_timestamp_wrapped_bound.yaml new file mode 100644 index 00000000000..5f1ae1a53a5 --- /dev/null +++ b/integ-test/src/test/resources/expectedOutput/ppl/explain_filter_push_timestamp_wrapped_bound.yaml @@ -0,0 +1,19 @@ +root: + name: ProjectOperator + description: + fields: "[account_number, firstname, address, birthdate, gender, city, lastname,\ + \ balance, employer, state, age, email, male]" + children: + - name: OpenSearchIndexScan + description: + request: "OpenSearchQueryRequest(indexName=opensearch-sql_test_index_bank,\ + \ sourceBuilder={\"from\":0,\"size\":10000,\"timeout\":\"1m\",\"query\"\ + :{\"bool\":{\"filter\":[{\"range\":{\"birthdate\":{\"from\":null,\"to\"\ + :1541721600000,\"include_lower\":true,\"include_upper\":false,\"boost\"\ + :1.0}}},{\"range\":{\"birthdate\":{\"from\":1481155200000,\"to\":null,\"\ + include_lower\":false,\"include_upper\":true,\"boost\":1.0}}}],\"adjust_pure_negative\"\ + :true,\"boost\":1.0}},\"_source\":{\"includes\":[\"account_number\",\"firstname\"\ + ,\"address\",\"birthdate\",\"gender\",\"city\",\"lastname\",\"balance\"\ + ,\"employer\",\"state\",\"age\",\"email\",\"male\"]}}, pitId=*,\ + \ cursorKeepAlive=1m, searchAfter=null, searchResponse=null)" + children: [] diff --git a/integ-test/src/test/resources/expectedOutput/ppl/explain_filter_push_timestamp_wrapped_field.yaml b/integ-test/src/test/resources/expectedOutput/ppl/explain_filter_push_timestamp_wrapped_field.yaml new file mode 100644 index 00000000000..5f1ae1a53a5 --- /dev/null +++ b/integ-test/src/test/resources/expectedOutput/ppl/explain_filter_push_timestamp_wrapped_field.yaml @@ -0,0 +1,19 @@ +root: + name: ProjectOperator + description: + fields: "[account_number, firstname, address, birthdate, gender, city, lastname,\ + \ balance, employer, state, age, email, male]" + children: + - name: OpenSearchIndexScan + description: + request: "OpenSearchQueryRequest(indexName=opensearch-sql_test_index_bank,\ + \ sourceBuilder={\"from\":0,\"size\":10000,\"timeout\":\"1m\",\"query\"\ + :{\"bool\":{\"filter\":[{\"range\":{\"birthdate\":{\"from\":null,\"to\"\ + :1541721600000,\"include_lower\":true,\"include_upper\":false,\"boost\"\ + :1.0}}},{\"range\":{\"birthdate\":{\"from\":1481155200000,\"to\":null,\"\ + include_lower\":false,\"include_upper\":true,\"boost\":1.0}}}],\"adjust_pure_negative\"\ + :true,\"boost\":1.0}},\"_source\":{\"includes\":[\"account_number\",\"firstname\"\ + ,\"address\",\"birthdate\",\"gender\",\"city\",\"lastname\",\"balance\"\ + ,\"employer\",\"state\",\"age\",\"email\",\"male\"]}}, pitId=*,\ + \ cursorKeepAlive=1m, searchAfter=null, searchResponse=null)" + children: [] diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/request/PredicateAnalyzer.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/request/PredicateAnalyzer.java index 476e0018fcd..5bb6693d90a 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/request/PredicateAnalyzer.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/request/PredicateAnalyzer.java @@ -249,6 +249,13 @@ public static QueryExpression analyzeExpression( } } + /** + * Date/time conversion operators, i.e. the ones that only reinterpret the value. A conversion to + * the type the field already has is a no-op and can be folded away; see {@link + * Visitor#isRedundantDateCastOverField}. + */ + private static final Set DATE_CONVERSION_OPERATORS = Set.of("TIMESTAMP", "DATE", "TIME"); + /** Traverses {@link RexNode} tree and builds OpenSearch query. */ static class Visitor extends RexVisitorImpl { @@ -365,6 +372,14 @@ static RexUnknownAs getNullAsForSearch(RexCall search) { @Override public Expression visitCall(RexCall call) { + // Fold a redundant date/time cast over a field of the same date/time type -- e.g. + // timestamp() or CAST( AS TIMESTAMP) -- to the bare field + // reference. Such a wrap is a no-op, so the enclosing comparison can push down to a native + // range query instead of falling back to a per-document script. + if (isRedundantDateCastOverField(call)) { + return visitInputRef((RexInputRef) call.getOperands().get(0)); + } + SqlSyntax syntax = call.getOperator().getSyntax(); if (!supportedRexCall(call)) { String message = format(Locale.ROOT, "Unsupported call: [%s]", call); @@ -1000,6 +1015,47 @@ private CastExpression toCastExpression(RexCall call) { return new CastExpression(call.getType(), argument); } + /** + * True if {@code call} is a date/time cast — a {@code CAST(... AS TIMESTAMP/DATE/TIME)} or the + * {@code timestamp()}/{@code date()}/{@code time()} builtins, both of which yield a date/time + * UDT — applied to a single field reference whose own type is the same date/time type. + * Such a wrap is a no-op, so it is redundant for a comparison and can be unwrapped to the bare + * field, letting the predicate push down instead of falling back to a per-document script. + * + *

The target type must match the field type exactly. A cast that changes the date/time type + * is a real conversion and must not be folded: {@code date()} truncates the + * time component (so {@code date(ts) <= '2024-01-15'} is not {@code ts <= '2024-01-15'}), and + * {@code time()} extracts the time of day, which is not even monotonic with + * respect to the timestamp. + */ + private boolean isRedundantDateCastOverField(RexCall call) { + if (call.getOperands().size() != 1) { + return false; + } + if (!(call.getOperands().get(0) instanceof RexInputRef inputRef)) { + return false; + } + // Only a cast or a date/time conversion operator can be a no-op. The result type alone is not + // sufficient: other single-argument functions also return a date/time type while changing the + // value (LAST_DAY being the clearest example), and folding those away would be wrong. + if (call.getKind() != SqlKind.CAST + && !DATE_CONVERSION_OPERATORS.contains( + call.getOperator().getName().toUpperCase(Locale.ROOT))) { + return false; + } + // Date/time values are modelled as UDTs (EXPR_DATE/EXPR_TIME/EXPR_TIMESTAMP) whose backing + // SqlTypeName is VARCHAR, so the UDT identifies the cast target -- not getSqlTypeName(). + if (!(call.getType() instanceof ExprSqlType exprSqlType)) { + return false; + } + ExprUDT udt = exprSqlType.getUdt(); + if (udt != ExprUDT.EXPR_TIMESTAMP && udt != ExprUDT.EXPR_DATE && udt != ExprUDT.EXPR_TIME) { + return false; + } + ExprType fieldType = new NamedFieldExpression(inputRef, schema, fieldTypes).getCoreExprType(); + return udt.getExprCoreType().equals(fieldType); + } + private static NamedFieldExpression toNamedField(RexLiteral literal) { return new NamedFieldExpression(literal); } @@ -1871,6 +1927,17 @@ boolean isTimeStampType() { : type.getOriginalExprType()); } + /** The field's underlying core type, unwrapping {@link OpenSearchDataType} if present. */ + @Nullable + ExprType getCoreExprType() { + if (type == null) { + return null; + } + return type.getOriginalExprType() instanceof OpenSearchDataType osType + ? osType.getExprCoreType() + : type.getOriginalExprType(); + } + boolean isTextType() { return type != null && type.getOriginalExprType() instanceof OpenSearchTextType; } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/LuceneQuery.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/LuceneQuery.java index 426af9a4b11..d30115dcfc8 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/LuceneQuery.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/LuceneQuery.java @@ -52,10 +52,11 @@ public abstract class LuceneQuery { * @return return true if supported, otherwise false. */ public boolean canSupport(FunctionExpression func) { - return (func.getArguments().size() == 2) - && (func.getArguments().get(0) instanceof ReferenceExpression) + return ((func.getArguments().size() == 2) + && (func.getArguments().get(0) instanceof ReferenceExpression + || referenceWrappedByRedundantDateCast(func.getArguments().get(0))) && (func.getArguments().get(1) instanceof LiteralExpression - || literalExpressionWrappedByCast(func)) + || literalExpressionWrappedByCast(func))) || isMultiParameterQuery(func); } @@ -86,15 +87,147 @@ private boolean isMultiParameterQuery(FunctionExpression func) { } /** - * Check if the second argument of the function is a literal expression wrapped by cast function. + * Check if the value operand of the function is a literal wrapped in conversions that can be + * evaluated up front, so the comparison can still push down instead of falling back to a script. */ protected boolean literalExpressionWrappedByCast(FunctionExpression func) { - if (func.getArguments().get(1) instanceof FunctionExpression) { - FunctionExpression expr = (FunctionExpression) func.getArguments().get(1); - return castMap.containsKey(expr.getFunctionName()) - && expr.getArguments().get(0) instanceof LiteralExpression; + return resolveLiteralOperand( + func.getArguments().get(1), fieldDateType(func.getArguments().get(0))) + != null; + } + + /** + * The date/time type of the filtered field, or null when it is not a date/time field. Used to + * keep a date conversion on the bound from being resolved against a field of a different + * date/time type, which would silently change the bound (for example resolving a {@code + * timestamp('...')} bound against a {@code DATE} field drops the time component). + */ + private ExprCoreType fieldDateType(Expression arg) { + Expression ref = + referenceWrappedByRedundantDateCast(arg) + ? ((FunctionExpression) arg).getArguments().get(0) + : arg; + return ref.type() instanceof OpenSearchDateType dateType ? dateType.getExprCoreType() : null; + } + + /** + * The {@code timestamp()}/{@code date()}/{@code time()} builtins and the cast they are equal to. + */ + private static final Map DATE_CONVERSION_TO_CAST = + ImmutableMap.of( + BuiltinFunctionName.TIMESTAMP.getName(), + BuiltinFunctionName.CAST_TO_TIMESTAMP.getName(), + BuiltinFunctionName.DATE.getName(), + BuiltinFunctionName.CAST_TO_DATE.getName(), + BuiltinFunctionName.TIME.getName(), + BuiltinFunctionName.CAST_TO_TIME.getName()); + + /** + * Resolve the value operand to the cast that should be evaluated and the literal to evaluate it + * on, or null when the operand is not a literal behind supported conversions. + * + *

Besides the plain {@code CAST_TO_*(literal)} form, this accepts the {@code + * timestamp()}/{@code date()}/{@code time()} builtins, which are what clients typically emit for + * a time-range bound (for example the Grafana OpenSearch data source builds its PPL time filter + * as {@code where `field` >= timestamp('...')}). PPL coerces the string argument first, so the + * operand arrives as {@code timestamp(cast_to_timestamp('...'))}; applying {@code timestamp()} to + * a value that is already a timestamp is a no-op, so the inner cast alone is evaluated. The cast + * is resolved rather than evaluated here so that {@link #castMap} keeps parsing the literal + * against the field's declared date formats. + */ + private Map.Entry resolveLiteralOperand( + Expression expr, ExprCoreType fieldDateType) { + if (!(expr instanceof FunctionExpression fn)) { + return null; + } + FunctionName name = fn.getFunctionName(); + Expression inner = fn.getArguments().isEmpty() ? null : fn.getArguments().get(0); + + if (castMap.containsKey(name) && inner instanceof LiteralExpression literal) { + return Map.entry(name, literal); + } + FunctionName equivalentCast = DATE_CONVERSION_TO_CAST.get(name); + if (equivalentCast == null || fn.getArguments().size() != 1) { + return null; + } + // Only resolve the conversion when it produces the type the field already has. Resolving across + // date/time types would re-format the bound into the field's format and change which documents + // match, so those are left on the script path. + if (!DATE_CAST_TARGET_TYPES.get(name).equals(fieldDateType)) { + return null; + } + if (inner instanceof LiteralExpression literal) { + return Map.entry(equivalentCast, literal); + } + if (inner instanceof FunctionExpression innerFn + && innerFn.getArguments().size() == 1 + && innerFn.getArguments().get(0) instanceof LiteralExpression literal + && castMap.containsKey(innerFn.getFunctionName()) + && DATE_CAST_TARGET_TYPES + .get(name) + .equals(DATE_CAST_TARGET_TYPES.get(innerFn.getFunctionName()))) { + return Map.entry(innerFn.getFunctionName(), literal); } - return false; + return null; + } + + /** Date/time cast functions mapped to the type each one produces. */ + private static final Map DATE_CAST_TARGET_TYPES = + ImmutableMap.builder() + .put(BuiltinFunctionName.CAST_TO_TIMESTAMP.getName(), ExprCoreType.TIMESTAMP) + .put(BuiltinFunctionName.TIMESTAMP.getName(), ExprCoreType.TIMESTAMP) + .put(BuiltinFunctionName.CAST_TO_DATE.getName(), ExprCoreType.DATE) + .put(BuiltinFunctionName.DATE.getName(), ExprCoreType.DATE) + .put(BuiltinFunctionName.CAST_TO_TIME.getName(), ExprCoreType.TIME) + .put(BuiltinFunctionName.TIME.getName(), ExprCoreType.TIME) + .build(); + + /** + * Check if the left operand is a date/time cast (or the {@code timestamp()}/{@code date()}/{@code + * time()} builtin) applied to a reference of the same date/time type. Such a wrap is a + * no-op, so it can be unwrapped, letting the predicate push down to a native range/term query + * instead of falling back to a per-document script. + * + *

The cast target must match the field type exactly. A cast that changes the date/time type is + * a real conversion and must not be folded: {@code date()} truncates the time + * component (so {@code date(ts) <= '2024-01-15'} is not {@code ts <= '2024-01-15'}), and {@code + * time()} extracts the time of day, which is not even monotonic with respect to + * the timestamp. + * + * @param arg left operand of the comparison. + * @return true if the operand is a redundant, order-preserving date/time cast over a reference. + */ + protected boolean referenceWrappedByRedundantDateCast(Expression arg) { + if (!(arg instanceof FunctionExpression)) { + return false; + } + FunctionExpression fn = (FunctionExpression) arg; + ExprCoreType castTarget = DATE_CAST_TARGET_TYPES.get(fn.getFunctionName()); + if (castTarget == null || fn.getArguments().size() != 1) { + return false; + } + Expression inner = fn.getArguments().get(0); + return inner instanceof ReferenceExpression + && inner.type() instanceof OpenSearchDateType dateType + && castTarget.equals(dateType.getExprCoreType()); + } + + /** + * Return the underlying reference of the left operand, unwrapping a redundant date/time cast if + * present (see {@link #referenceWrappedByRedundantDateCast}). Callers must ensure {@link + * #canSupport} returned true for the enclosing function; otherwise an {@link + * IllegalStateException} is thrown rather than allowing an unchecked cast to fail. + */ + private ReferenceExpression unwrapReference(Expression arg) { + if (arg instanceof ReferenceExpression) { + return (ReferenceExpression) arg; + } + if (referenceWrappedByRedundantDateCast(arg)) { + return (ReferenceExpression) ((FunctionExpression) arg).getArguments().get(0); + } + throw new IllegalStateException( + "Left operand must be a reference or a redundant date/time cast over a reference; " + + "canSupport() must be checked before build()"); } /** @@ -105,7 +238,7 @@ protected boolean literalExpressionWrappedByCast(FunctionExpression func) { * @return query */ public QueryBuilder build(FunctionExpression func) { - ReferenceExpression ref = (ReferenceExpression) func.getArguments().get(0); + ReferenceExpression ref = unwrapReference(func.getArguments().get(0)); Expression expr = func.getArguments().get(1); ExprValue literalValue = expr instanceof LiteralExpression ? expr.valueOf() : cast((FunctionExpression) expr, ref); @@ -117,9 +250,14 @@ public QueryBuilder build(FunctionExpression func) { } private ExprValue cast(FunctionExpression castFunction, ReferenceExpression ref) { - return castMap - .get(castFunction.getFunctionName()) - .apply((LiteralExpression) castFunction.getArguments().get(0), ref); + Map.Entry resolved = + resolveLiteralOperand(castFunction, fieldDateType(ref)); + if (resolved == null) { + throw new IllegalStateException( + "Value operand must be a literal behind supported conversions; canSupport() must be" + + " checked before build()"); + } + return castMap.get(resolved.getKey()).apply(resolved.getValue(), ref); } /** Type converting map. */ diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/request/PredicateAnalyzerTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/request/PredicateAnalyzerTest.java index 01c5e6108ef..baeb7f0e989 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/request/PredicateAnalyzerTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/request/PredicateAnalyzerTest.java @@ -1304,6 +1304,85 @@ void gte_generatesRangeQueryWithFormatForDateTime() throws ExpressionNotAnalyzab result.toString()); } + @Test + void gte_redundantTimestampCastOverTimestampField_generatesRangeQuery() + throws ExpressionNotAnalyzableException { + // timestamp() is a no-op wrap, so the comparison must still push down to a + // native range query instead of falling back to a per-document script. + RexNode wrapped = PPLFuncImpTable.INSTANCE.resolve(builder, "timestamp", field4); + RexNode call = + builder.makeCall(SqlStdOperatorTable.GREATER_THAN_OR_EQUAL, wrapped, dateTimeLiteral); + QueryBuilder result = PredicateAnalyzer.analyze(call, schema, fieldTypes); + + assertInstanceOf(RangeQueryBuilder.class, result); + assertEquals( + """ + { + "range" : { + "d" : { + "from" : "1987-02-03T04:34:56.000Z", + "to" : null, + "include_lower" : true, + "include_upper" : true, + "format" : "date_time", + "boost" : 1.0 + } + } + }\ + """, + result.toString()); + } + + @Test + void equals_lastDayOverTimestampField_isNotFoldedAndFallsBackToScript() + throws ExpressionNotAnalyzableException { + // LAST_DAY takes a single date/time argument and returns a date/time type, but it changes the + // value, so it must not be treated as a redundant conversion. Only CAST and the + // timestamp()/date()/time() conversion operators are foldable. + final RelDataType rowType = + builder + .getTypeFactory() + .builder() + .kind(StructKind.FULLY_QUALIFIED) + .add("a", builder.getTypeFactory().createSqlType(SqlTypeName.BIGINT)) + .add("b", builder.getTypeFactory().createSqlType(SqlTypeName.VARCHAR)) + .add("c", builder.getTypeFactory().createSqlType(SqlTypeName.VARCHAR)) + .add("d", typeFactory.createUDT(ExprUDT.EXPR_TIMESTAMP)) + .build(); + RexNode wrapped = PPLFuncImpTable.INSTANCE.resolve(builder, "last_day", field4); + RexNode call = builder.makeCall(SqlStdOperatorTable.EQUALS, wrapped, dateTimeLiteral); + QueryBuilder result = + PredicateAnalyzer.analyzeExpression(call, schema, fieldTypes, rowType, cluster).builder(); + + assertInstanceOf(ScriptQueryBuilder.class, result); + } + + @Test + void lte_dateCastOverTimestampField_isNotFoldedAndFallsBackToScript() + throws ExpressionNotAnalyzableException { + // date() truncates the time component, so it is NOT a redundant wrap and must + // not be folded to the bare field: `date(ts) <= '1987-02-03'` is not `ts <= '1987-02-03'`. + // It therefore stays on the script path rather than becoming a range query. + final RelDataType rowType = + builder + .getTypeFactory() + .builder() + .kind(StructKind.FULLY_QUALIFIED) + .add("a", builder.getTypeFactory().createSqlType(SqlTypeName.BIGINT)) + .add("b", builder.getTypeFactory().createSqlType(SqlTypeName.VARCHAR)) + .add("c", builder.getTypeFactory().createSqlType(SqlTypeName.VARCHAR)) + .add("d", typeFactory.createUDT(ExprUDT.EXPR_TIMESTAMP)) + .build(); + RexNode wrapped = PPLFuncImpTable.INSTANCE.resolve(builder, "date", field4); + RexNode dateLiteral = + builder.makeLiteral("1987-02-03", typeFactory.createUDT(ExprUDT.EXPR_DATE), true); + RexNode call = builder.makeCall(SqlStdOperatorTable.LESS_THAN_OR_EQUAL, wrapped, dateLiteral); + QueryBuilder result = + PredicateAnalyzer.analyzeExpression(call, schema, fieldTypes, rowType, cluster).builder(); + + assertInstanceOf(ScriptQueryBuilder.class, result); + } + @Test void isTrue_booleanField_generatesTermQuery() throws ExpressionNotAnalyzableException { // IS_TRUE(boolean_field) should generate a term query with value true diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/FilterQueryBuilderTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/FilterQueryBuilderTest.java index e930056474a..1efe0efa5a0 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/FilterQueryBuilderTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/FilterQueryBuilderTest.java @@ -6,6 +6,7 @@ package org.opensearch.sql.opensearch.storage.script.filter; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; @@ -53,6 +54,7 @@ import org.opensearch.sql.expression.LiteralExpression; import org.opensearch.sql.expression.ReferenceExpression; import org.opensearch.sql.opensearch.data.type.OpenSearchDataType; +import org.opensearch.sql.opensearch.data.type.OpenSearchDateType; import org.opensearch.sql.opensearch.data.type.OpenSearchTextType; import org.opensearch.sql.opensearch.storage.serde.ExpressionSerializer; @@ -158,6 +160,110 @@ void should_build_range_query_for_comparison_expression() { buildQuery(expr))); } + @Test + void should_push_down_range_query_when_date_field_wrapped_by_redundant_date_cast() { + // Wrapping an already date-typed field in timestamp()/CAST(... AS TIMESTAMP) is redundant and + // range-preserving, so `timestamp() >= ` must push down to a native range + // query rather than fall back to a per-document script. + OpenSearchDateType dateType = OpenSearchDateType.of(TIMESTAMP); + Expression[] predicates = { + DSL.gte( + DSL.timestamp(ref("datetime", dateType)), + DSL.castTimestamp(literal("2021-11-08 17:00:00"))), + DSL.gte( + DSL.castTimestamp(ref("datetime", dateType)), + DSL.castTimestamp(literal("2021-11-08 17:00:00"))) + }; + for (Expression predicate : predicates) { + String query = buildQuery(predicate); + assertTrue(query.contains("\"range\""), query); + assertTrue(query.contains("datetime"), query); + assertFalse(query.contains("script"), query); + } + } + + @Test + void should_push_down_range_query_when_bound_wrapped_by_timestamp_builtin() { + // `field >= timestamp('...')` is the shape clients emit for a time-range bound (e.g. the + // Grafana OpenSearch data source's PPL time filter). PPL coerces the string argument first, so + // the operand arrives as timestamp(cast_to_timestamp('...')). Both forms must push down to a + // native range rather than falling back to a per-document script. + OpenSearchDateType dateType = OpenSearchDateType.of(TIMESTAMP); + Expression[] predicates = { + DSL.gte(ref("datetime", dateType), DSL.timestamp(literal("2021-11-08 17:00:00"))), + DSL.gte( + ref("datetime", dateType), + DSL.timestamp(DSL.castTimestamp(literal("2021-11-08 17:00:00")))) + }; + for (Expression predicate : predicates) { + String query = buildQuery(predicate); + assertTrue(query.contains("\"range\""), query); + assertTrue(query.contains("datetime"), query); + assertFalse(query.contains("script"), query); + } + } + + @Test + void should_not_push_down_when_bound_conversion_changes_the_date_type() { + // date(cast_to_timestamp(...)) truncates, so the outer conversion is not a no-op over the inner + // cast and the bound is not resolved up front. + mockToStringSerializer(); + String query = + buildQuery( + DSL.gte( + ref("datetime", OpenSearchDateType.of(TIMESTAMP)), + DSL.date(DSL.castTimestamp(literal("2021-11-08 17:00:00"))))); + assertTrue(query.contains("script"), query); + assertFalse(query.contains("\"range\""), query); + } + + @Test + void should_not_resolve_bound_conversion_of_a_different_type_than_the_field() { + // A timestamp() bound against a DATE field is not a no-op: resolving it would re-format the + // bound into the field's date-only format and drop the time component, changing which documents + // match. Such a bound stays on the script path. + mockToStringSerializer(); + String query = + buildQuery( + DSL.gte( + ref("datefield", OpenSearchDateType.of(DATE)), + DSL.timestamp(literal("2021-11-08 17:00:00")))); + assertTrue(query.contains("script"), query); + assertFalse(query.contains("\"range\""), query); + } + + @Test + void should_not_push_down_when_date_cast_changes_the_date_type() { + // date()/time() over a timestamp field are real conversions, not no-ops: date() truncates the + // time component and time() extracts the time of day (not monotonic in the timestamp), so they + // must keep using the script path rather than being folded to the bare field. + mockToStringSerializer(); + OpenSearchDateType dateType = OpenSearchDateType.of(TIMESTAMP); + Expression[] predicates = { + DSL.lte(DSL.date(ref("datetime", dateType)), DSL.castDate(literal("2021-11-08"))), + DSL.gte(DSL.time(ref("datetime", dateType)), DSL.castTime(literal("17:00:00"))) + }; + for (Expression predicate : predicates) { + String query = buildQuery(predicate); + assertTrue(query.contains("script"), query); + assertFalse(query.contains("\"range\""), query); + } + } + + @Test + void should_not_push_down_when_date_cast_wraps_non_date_field() { + // Casting a non-date field to a timestamp is a real, not necessarily order-preserving, + // conversion, so it must stay on the script path (the fold only applies to date-typed fields). + mockToStringSerializer(); + String query = + buildQuery( + DSL.gte( + DSL.castTimestamp(ref("string_value", STRING)), + DSL.castTimestamp(literal("2021-11-08 17:00:00")))); + assertTrue(query.contains("script"), query); + assertFalse(query.contains("\"range\""), query); + } + @Test void should_build_wildcard_query_for_like_expression() { assertJsonEquals(