Skip to content

Render date-only and time-only analytics-engine columns as SQL DATE / TIME#5524

Draft
ahkcs wants to merge 2 commits into
opensearch-project:mainfrom
ahkcs:analytics-date-type
Draft

Render date-only and time-only analytics-engine columns as SQL DATE / TIME#5524
ahkcs wants to merge 2 commits into
opensearch-project:mainfrom
ahkcs:analytics-date-type

Conversation

@ahkcs

@ahkcs ahkcs commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

Description

Companion SQL-plugin side of DATE + TIME logical-type support on the analytics-engine route. Pairs with opensearch-project/OpenSearch#22062, which adds DateType / TimeType Calcite UDTs to analytics-api and emits them for date-only / time-only-format columns.

This change consumes those UDTs in the two SQL-plugin dispatch points that already special-case IpType / BinaryType:

  • OpenSearchTypeFactory.convertAnalyticsEngineRelDataTypeToExprType — maps a DateType column to ExprCoreType.DATE and a TimeType column to ExprCoreType.TIME (the response schema label).
  • AnalyticsExecutionEngine.toExprValue — when the column is DateType / TimeType, materializes the cell as an ExprDateValue / ExprTimeValue by truncating the timestamp result to a LocalDate / LocalTime (epoch interpreted at UTC, matching OpenSearch storage).

Net effect on the analytics route: a date-only field renders as a SQL DATE (1984-04-12) and a time-only field as a SQL TIME (09:00:00), instead of a widened TIMESTAMP, matching the v2 / Calcite path. Datetime columns are unchanged.

Pass rate (analytics route, -Dtests.analytics.parquet_indices=true)

Test before after
CalcitePPLAggregationIT.testCountByDateTypeSpanForDifferentFormats (+paginating)
CalcitePPLAggregationIT.testCountByDateTypeSpanWithDifferentUnits (+paginating)
CalcitePPLAggregationIT.testCountByTimeTypeSpanForDifferentFormats
CalcitePPLAggregationIT.testCountByTimeTypeSpanWithDifferentUnits
CalcitePPLAggregationIT.testCountByNullableTimeSpan
CalcitePPLAggregationIT.testCountBySpanForCustomFormats
CalciteAnalyticsDatetimeWireFormatIT (date + time cols)

CalciteAnalyticsDatetimeWireFormatIT.testDateRootColumnYmdFormat / testTimeRootColumnHmsFormat previously asserted the widened-timestamp behavior; they now expect date / time. The v2 / Calcite path is unaffected (this code path is analytics-route only). No regressions — only columns whose mapping format is date-only or time-only change type.

Dependency

Compiles once OpenSearch#22062 is published to the org.opensearch.sandbox:analytics-api snapshot (the DateType / TimeType classes), the same cross-repo coupling the existing IpType / BinaryType consumers have. Do not merge before #22062 lands and the snapshot is bumped.

Check List

  • New functionality includes testing.
  • Commits are signed per the DCO using --signoff.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 6a14c48)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 6a14c48
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Handle potential overflow in epoch conversion

Consider handling potential ArithmeticException when converting Number to long. If
the number exceeds Long.MAX_VALUE or Long.MIN_VALUE, longValue() may produce
incorrect results through overflow, leading to invalid dates.

core/src/main/java/org/opensearch/sql/executor/analytics/AnalyticsExecutionEngine.java [288-290]

 if (value instanceof Number number) {
-  return Instant.ofEpochMilli(number.longValue()).atZone(ZoneOffset.UTC).toLocalDate();
+  try {
+    long epochMilli = number.longValue();
+    return Instant.ofEpochMilli(epochMilli).atZone(ZoneOffset.UTC).toLocalDate();
+  } catch (DateTimeException e) {
+    return null;
+  }
 }
Suggestion importance[1-10]: 3

__

Why: While the suggestion correctly identifies a potential issue with Number to long conversion, the proposed solution doesn't address the actual overflow problem (which occurs in longValue(), not ofEpochMilli()). The DateTimeException catch is also unlikely to trigger for overflow cases. The suggestion has limited practical impact since overflow scenarios are rare in typical date handling.

Low

Previous suggestions

Suggestions up to commit 88df0b3
CategorySuggestion                                                                                                                                    Impact
Possible issue
Validate number type before conversion

Converting arbitrary Number types to epoch milliseconds may cause precision loss or
incorrect results for non-integral numbers (e.g., Double, Float). Consider
validating that the number is an integral type or handling floating-point numbers
explicitly to avoid silent data corruption.

core/src/main/java/org/opensearch/sql/executor/analytics/AnalyticsExecutionEngine.java [277-279]

 if (value instanceof Number number) {
+  if (number instanceof Double || number instanceof Float) {
+    throw new IllegalArgumentException("Floating-point numbers are not supported for date conversion");
+  }
   return Instant.ofEpochMilli(number.longValue()).atZone(ZoneOffset.UTC).toLocalDate();
 }
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies a potential issue with converting floating-point numbers to epoch milliseconds, which could lead to precision loss. However, the severity is moderate since the method already returns null for unrecognized types as a fallback, and the likelihood of receiving floating-point epoch values in this context may be low.

Low

ahkcs added 2 commits June 8, 2026 13:55
Consumes the DateType UDT added in opensearch-project/OpenSearch (analytics-api):
when the analytics-engine catalog types a date-only column as DateType,
OpenSearchTypeFactory.convertAnalyticsEngineRelDataTypeToExprType now maps it to
ExprCoreType.DATE (the response schema label), and AnalyticsExecutionEngine
materializes the cell as an ExprDateValue by truncating the timestamp result to a
LocalDate. Mirrors the existing IpType / BinaryType dispatch in both classes.

This flips date-only fields on the analytics route from TIMESTAMP (1984-04-12
00:00:00) to DATE (1984-04-12), matching the v2 / Calcite path. Updates
CalciteAnalyticsDatetimeWireFormatIT.testDateRootColumnYmdFormat, which previously
asserted the widened-timestamp behavior, to expect the date type and value.

Depends on the analytics-api DateType class; compiles once that change is
published to the snapshot repo (same cross-repo pattern as IpType/BinaryType).

Signed-off-by: Kai Huang <ahkcs@amazon.com>
Phase 2, extending the DateType consumer. Maps a TimeType column to
ExprCoreType.TIME in OpenSearchTypeFactory.convertAnalyticsEngineRelDataTypeToExprType
and materializes the cell as an ExprTimeValue (timestamp result truncated to a
LocalTime) in AnalyticsExecutionEngine. Mirrors the DateType dispatch.

Flips time-only fields on the analytics route from TIMESTAMP (1970-01-01
09:00:00) to TIME (09:00:00), matching the v2 / Calcite path. Updates
CalciteAnalyticsDatetimeWireFormatIT.testTimeRootColumnHmsFormat, which previously
asserted the widened-timestamp behavior, to expect the time type and value.

Depends on the analytics-api TimeType class (opensearch-project/OpenSearch#22062).

Signed-off-by: Kai Huang <ahkcs@amazon.com>
@ahkcs ahkcs force-pushed the analytics-date-type branch from 88df0b3 to 6a14c48 Compare June 8, 2026 20:57
@ahkcs ahkcs changed the title Render date-only analytics-engine columns as SQL DATE (Phase 1) Render date-only and time-only analytics-engine columns as SQL DATE / TIME Jun 8, 2026
@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 6a14c48

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant