Skip to content

Commit c7b171d

Browse files
mengweiericmengweieric
committed
Remove DatetimeOutputCastRule on the analytics-engine route (#5420)
On the analytics-engine route, every datetime root column is wrapped in `CAST(<DATE/TIME/TIMESTAMP> AS VARCHAR)` by `DatetimeOutputCastRule`. A matching `DatetimeOutputCastRewriter` on the engine side then translates that cast into DataFusion's `to_char` extension. Whenever the rewriter's format string and the PPL formatter disagree (e.g. trailing `Z`, `T` separator), users see wire-format divergence — issue #5420. Drop the cast rule and let the engine return real datetime cells. The PPL response pipeline already handles datetime → string conversion natively at the formatter layer: - `AnalyticsExecutionEngine.convertRows` feeds engine cells (`LocalDateTime` / `LocalDate` / `LocalTime`) through `ExprValueUtils.fromObjectValue`, which produces `ExprTimestampValue` / `ExprDateValue` / `ExprTimeValue`. - Their `value()` methods render the documented PPL space-separated format (`yyyy-MM-dd HH:mm:ss[.SSSSSSSSS]` etc.). The companion change in `opensearch-project/OpenSearch` removes the matching `DatetimeOutputCastRewriter` and its callsites. - Delete `DatetimeOutputCastRule`. - Drop the unused `UdtMapping.isDatetimeType` helper. - Rewrite the four cast-shape assertions in `DatetimeExtensionTest` to assert the post-removal RelNode (no `CAST(... VARCHAR)` wrapper, schema reports `DATE` / `TIME` / `TIMESTAMP`). Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
1 parent c4cac2a commit c7b171d

5 files changed

Lines changed: 38 additions & 113 deletions

File tree

‎api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeExtension.java‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,13 @@
1717
import org.opensearch.sql.calcite.type.AbstractExprRelDataType;
1818
import org.opensearch.sql.calcite.utils.OpenSearchTypeFactory.ExprUDT;
1919

20-
/** Datetime language extension that normalizes UDT types and casts output for wire-format. */
20+
/** Datetime language extension that normalizes datetime UDT types to standard Calcite types. */
2121
public class DatetimeExtension implements LanguageExtension {
2222

2323
@Override
2424
public List<RelShuttle> postAnalysisRules() {
25-
// Fresh instances per plan() because RelHomogeneousShuttle inherits a stateful stack.
26-
return List.of(new DatetimeUdtNormalizeRule(), new DatetimeOutputCastRule());
25+
// Fresh instance per plan() because RelHomogeneousShuttle inherits a stateful stack.
26+
return List.of(new DatetimeUdtNormalizeRule());
2727
}
2828

2929
/** Maps datetime UDT types to their standard Calcite equivalents. */
@@ -45,10 +45,5 @@ static Optional<UdtMapping> fromUdtType(RelDataType type) {
4545
ExprUDT udt = e.getUdt();
4646
return Arrays.stream(values()).filter(u -> u.udtType == udt).findFirst();
4747
}
48-
49-
/** Returns true if the given SqlTypeName is a standard datetime type. */
50-
static boolean isDatetimeType(SqlTypeName typeName) {
51-
return Arrays.stream(values()).anyMatch(u -> u.stdType == typeName);
52-
}
5348
}
5449
}

‎api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeOutputCastRule.java‎

Lines changed: 0 additions & 62 deletions
This file was deleted.

‎api/src/test/java/org/opensearch/sql/api/spec/datetime/DatetimeExtensionSqlTest.java‎

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,8 @@
2020
import org.opensearch.sql.executor.QueryType;
2121

2222
/**
23-
* Tests that DatetimeExtension post-analysis rules (UDT normalization and output VARCHAR cast)
24-
* apply correctly to the SQL V2 parser path through CalciteRelNodeVisitor.
23+
* Tests that the DatetimeExtension UDT-normalization post-analysis rule applies correctly to the
24+
* SQL V2 parser path through CalciteRelNodeVisitor.
2525
*/
2626
public class DatetimeExtensionSqlTest extends UnifiedQueryTestBase {
2727

@@ -47,7 +47,7 @@ protected Map<String, Table> getTableMap() {
4747
private Table createEventsTable() {
4848
return SimpleTable.builder()
4949
.col("id", INTEGER)
50-
.col("name", VARCHAR)
50+
.col("event_str", VARCHAR)
5151
.col("hire_date", DATE)
5252
.col("start_time", TIME)
5353
.col("created_at", TIMESTAMP)
@@ -57,12 +57,11 @@ private Table createEventsTable() {
5757
}
5858

5959
@Test
60-
public void testAllStandardDatetimeTypesCastToVarchar() {
60+
public void testStandardDatetimeTypesNotWrapped() {
6161
givenQuery("SELECT * FROM catalog.events")
6262
.assertPlan(
6363
"""
64-
LogicalProject(id=[$0], name=[$1], hire_date=[CAST($2):VARCHAR NOT NULL], start_time=[CAST($3):VARCHAR NOT NULL], created_at=[CAST($4):VARCHAR NOT NULL])
65-
LogicalTableScan(table=[[catalog, events]])
64+
LogicalTableScan(table=[[catalog, events]])
6665
""");
6766
}
6867

@@ -71,21 +70,19 @@ public void testFilterWithTimestampLiteral() {
7170
givenQuery("SELECT * FROM catalog.events WHERE created_at > '2024-01-01T00:00:00Z'")
7271
.assertPlan(
7372
"""
74-
LogicalProject(id=[$0], name=[$1], hire_date=[CAST($2):VARCHAR NOT NULL], start_time=[CAST($3):VARCHAR NOT NULL], created_at=[CAST($4):VARCHAR NOT NULL])
75-
LogicalFilter(condition=[>($4, TIMESTAMP('2024-01-01T00:00:00Z':VARCHAR))])
76-
LogicalTableScan(table=[[catalog, events]])
73+
LogicalFilter(condition=[>($4, TIMESTAMP('2024-01-01T00:00:00Z':VARCHAR))])
74+
LogicalTableScan(table=[[catalog, events]])
7775
""")
7876
.assertReturnType("TIMESTAMP", TIMESTAMP, 9);
7977
}
8078

8179
@Test
8280
public void testComparisonWithDatetimeUdf() {
83-
givenQuery("SELECT * FROM catalog.events WHERE created_at < DATE(name)")
81+
givenQuery("SELECT * FROM catalog.events WHERE created_at < DATE(event_str)")
8482
.assertPlan(
8583
"""
86-
LogicalProject(id=[$0], name=[$1], hire_date=[CAST($2):VARCHAR NOT NULL], start_time=[CAST($3):VARCHAR NOT NULL], created_at=[CAST($4):VARCHAR NOT NULL])
87-
LogicalFilter(condition=[<($4, TIMESTAMP(DATE($1)))])
88-
LogicalTableScan(table=[[catalog, events]])
84+
LogicalFilter(condition=[<($4, TIMESTAMP(DATE($1)))])
85+
LogicalTableScan(table=[[catalog, events]])
8986
""")
9087
.assertReturnType("DATE", DATE)
9188
.assertReturnType("TIMESTAMP", TIMESTAMP, 9);

‎api/src/test/java/org/opensearch/sql/api/spec/datetime/DatetimeExtensionTest.java‎

Lines changed: 22 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ public void setUp() {
5353
private Table createEventsTable() {
5454
return SimpleTable.builder()
5555
.col("id", INTEGER)
56-
.col("name", VARCHAR)
56+
.col("event_str", VARCHAR)
5757
.col("hire_date", DATE)
5858
.col("start_time", TIME)
5959
.col("created_at", TIMESTAMP)
@@ -63,18 +63,17 @@ private Table createEventsTable() {
6363
}
6464

6565
@Test
66-
public void testUdfResultNormalizedAndCastToVarchar() {
66+
public void testUdfResultNormalized() {
6767
givenQuery(
6868
"""
6969
source = catalog.events \
70-
| eval d = DATE(name), t = TIME(name), ts = TIMESTAMP(name) \
70+
| eval d = DATE(event_str), t = TIME(event_str), ts = TIMESTAMP(event_str) \
7171
| fields d, t, ts\
7272
""")
7373
.assertPlan(
7474
"""
75-
LogicalProject(d=[CAST($0):VARCHAR], t=[CAST($1):VARCHAR], ts=[CAST($2):VARCHAR])
76-
LogicalProject(d=[DATE($1)], t=[TIME($1)], ts=[TIMESTAMP($1)])
77-
LogicalTableScan(table=[[catalog, events]])
75+
LogicalProject(d=[DATE($1)], t=[TIME($1)], ts=[TIMESTAMP($1)])
76+
LogicalTableScan(table=[[catalog, events]])
7877
""")
7978
.assertReturnType("DATE", DATE)
8079
.assertReturnType("TIME", TIME, 9)
@@ -83,7 +82,9 @@ public void testUdfResultNormalizedAndCastToVarchar() {
8382

8483
@Test
8584
public void testNestedUdfCallsNormalized() {
86-
givenQuery("source = catalog.events | eval d = DATEDIFF(DATE(name), DATE(name)) | fields d")
85+
givenQuery(
86+
"source = catalog.events | eval d = DATEDIFF(DATE(event_str), DATE(event_str)) | fields"
87+
+ " d")
8788
.assertPlan(
8889
"""
8990
LogicalProject(d=[DATEDIFF(DATE($1), DATE($1))])
@@ -94,13 +95,12 @@ public void testNestedUdfCallsNormalized() {
9495
}
9596

9697
@Test
97-
public void testDateLiteralCastToVarchar() {
98+
public void testDateLiteralNormalized() {
9899
givenQuery("source = catalog.events | eval d = DATE('2024-01-01') | fields d")
99100
.assertPlan(
100101
"""
101-
LogicalProject(d=[CAST($0):VARCHAR])
102-
LogicalProject(d=[DATE('2024-01-01':VARCHAR)])
103-
LogicalTableScan(table=[[catalog, events]])
102+
LogicalProject(d=[DATE('2024-01-01':VARCHAR)])
103+
LogicalTableScan(table=[[catalog, events]])
104104
""")
105105
.assertReturnType("DATE", DATE);
106106
}
@@ -122,7 +122,7 @@ public void testFilterWithTimestampLiteral() {
122122

123123
@Test
124124
public void testComparisonWithDatetimeUdf() {
125-
givenQuery("source = catalog.events | where created_at < DATE(name) | fields id")
125+
givenQuery("source = catalog.events | where created_at < DATE(event_str) | fields id")
126126
.assertPlan(
127127
"""
128128
LogicalProject(id=[$0])
@@ -134,22 +134,21 @@ public void testComparisonWithDatetimeUdf() {
134134
}
135135

136136
@Test
137-
public void testAllStandardDatetimeTypesCastToVarchar() {
137+
public void testStandardDatetimeFieldsNotWrapped() {
138138
givenQuery("source = catalog.events | fields hire_date, start_time, created_at")
139139
.assertPlan(
140140
"""
141-
LogicalProject(hire_date=[CAST($0):VARCHAR NOT NULL], start_time=[CAST($1):VARCHAR NOT NULL], created_at=[CAST($2):VARCHAR NOT NULL])
142-
LogicalProject(hire_date=[$2], start_time=[$3], created_at=[$4])
143-
LogicalTableScan(table=[[catalog, events]])
141+
LogicalProject(hire_date=[$2], start_time=[$3], created_at=[$4])
142+
LogicalTableScan(table=[[catalog, events]])
144143
""");
145144
}
146145

147146
@Test
148147
public void testNonDatetimeFieldsNotWrapped() {
149-
givenQuery("source = catalog.events | fields id, name")
148+
givenQuery("source = catalog.events | fields id, event_str")
150149
.assertPlan(
151150
"""
152-
LogicalProject(id=[$0], name=[$1])
151+
LogicalProject(id=[$0], event_str=[$1])
153152
LogicalTableScan(table=[[catalog, events]])
154153
""");
155154
}
@@ -168,27 +167,24 @@ public void testSequentialPlanCallsDoNotCorruptShuttleStack() {
168167
+ " | stats count() as field_count, distinct_count(created_at) as distinct_count");
169168
planner.plan(
170169
"source = catalog.events"
171-
+ " | eval ts = TIMESTAMP(name)"
170+
+ " | eval ts = TIMESTAMP(event_str)"
172171
+ " | stats count() as field_count, distinct_count(ts) as distinct_count");
173172
planner.plan(
174173
"source = catalog.events | where created_at > \"2024-01-01\" | fields hire_date");
175174
}
176175
}
177176

178177
@Test
179-
public void testOutputCastCanCompileAndExecute() throws Exception {
178+
public void testDatetimeFieldsPreserveStandardTypes() throws Exception {
180179
RelNode plan =
181180
planner.plan("source = catalog.events | fields hire_date, start_time, created_at");
182181
try (PreparedStatement statement = compiler.compile(plan)) {
183182
ResultSet resultSet = statement.executeQuery();
184183
verify(resultSet)
185184
.expectSchema(
186-
col("hire_date", java.sql.Types.VARCHAR),
187-
col("start_time", java.sql.Types.VARCHAR),
188-
col("created_at", java.sql.Types.VARCHAR))
189-
.expectData(
190-
row("2024-01-16", "12:00:00", "2024-01-15 08:00:00"),
191-
row("2024-06-20", "14:00:00", "2024-06-20 00:00:00"));
185+
col("hire_date", java.sql.Types.DATE),
186+
col("start_time", java.sql.Types.TIME),
187+
col("created_at", java.sql.Types.TIMESTAMP));
192188
}
193189
}
194190
}

‎integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePlannerConcurrencyIT.java‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,10 +31,9 @@
3131
*
3232
* <p>The test methods here fire many queries through a thread pool to exercise concurrent {@code
3333
* plan()} invocations. New planner-level concurrency / state-isolation regressions belong in this
34-
* class. The current cases cover {@code DatetimeExtension}'s {@code RelHomogeneousShuttle}
35-
* subclasses ({@code DatetimeUdtNormalizeRule}, {@code DatetimeOutputCastRule}) which were
36-
* previously returned as static {@code INSTANCE}s and caused the production failure that motivated
37-
* this suite.
34+
* class. The current cases cover {@code DatetimeExtension}'s {@code RelHomogeneousShuttle} subclass
35+
* {@code DatetimeUdtNormalizeRule}, which was previously returned as a static {@code INSTANCE} and
36+
* caused the production failure that motivated this suite.
3837
*
3938
* <p>Run via:
4039
*

0 commit comments

Comments
 (0)