Skip to content

update datetime tests to stay within ae bounds#5534

Merged
Swiddis merged 1 commit into
opensearch-project:mainfrom
Swiddis:fix/datetime-ae-tests
Jun 10, 2026
Merged

update datetime tests to stay within ae bounds#5534
Swiddis merged 1 commit into
opensearch-project:mainfrom
Swiddis:fix/datetime-ae-tests

Conversation

@Swiddis

@Swiddis Swiddis commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Description

Simplifies the test indices & changes some values, preserves all the same core semantics of the test. Parquet/Datafusion use 64-bit microsecond epochs which cap at year 2262.

Related Issues

N/A

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added.
  • New PPL command checklist all confirmed.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff or -s.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Use more reasonable future dates

The timestamp '2242-12-15' is still far in the future and may exceed reasonable test
bounds. Consider using a date closer to the present (e.g., within 50-100 years) to
ensure test stability and avoid potential overflow issues in date handling systems.

integ-test/src/test/java/org/opensearch/sql/ppl/DateTimeComparisonIT.java [234]

-$("TIME('09:07:00') < TIMESTAMP('2242-12-15 22:15:07')", "t_ts_t", true),
+$("TIME('09:07:00') < TIMESTAMP('2070-12-15 22:15:07')", "t_ts_t", true),
Suggestion importance[1-10]: 5

__

Why: The suggestion to use dates closer to the present (e.g., 2070 instead of 2242) is reasonable for test stability, though the year 2242 is still within the valid range for most date systems. The change would make tests more conservative and potentially avoid edge cases, but the existing date is not technically incorrect.

Low

@RyanL1997 RyanL1997 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the change @Swiddis.

one small nit on the description — the i64 cap that kicks in at year 2262 is from nanosecond precision, not microsecond (μs would actually let us go way past year 290k). doesn't change anything about the fix itself since 2242 is under 2262 either way.

public void init() throws Exception {
super.init();
loadIndex(Index.DATA_TYPE_NONNUMERIC);
loadIndex(Index.DATETIME_SIMPLE);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

qq: does the sql variant's testCompare actually need this index loaded? scrolling down to the method body, it is just select <expr> AS <name> with no FROM clause, so i don't think this load is reaching any query.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

lol, probably. i think it's just copied from ppl maybe since ppl doesn't support bare queries without indices. won't rock the boat with it though

@Swiddis Swiddis merged commit 8cce7a8 into opensearch-project:main Jun 10, 2026
61 of 64 checks passed
@Swiddis Swiddis deleted the fix/datetime-ae-tests branch June 10, 2026 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants