Skip to content

Fix issue 2489 #3442

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.

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
merged 7 commits into from
Apr 22, 2025
Merged

Fix issue 2489 #3442

merged 7 commits into from
Apr 22, 2025

Conversation

penghuo
Copy link
Collaborator

@penghuo penghuo commented Mar 18, 2025

Description

  • Use OpenSearch DateFieldMapper.getDefaultDateTimeFormatter as default format if format is missing.
  • Integrate with yamlRestTest to framework to test OpenSearch specific cases.
    • YamlRestTest execut along with ./gradlew integ-test:build.
    • YamlRestTest execut independenly. ./gradlew integ-test:yamlRestTest.
  • Remove "2015-01-01 12:10:30" related UT, it is not valid OpenSearch default date format.
### Create Index
PUT {{baseUrl}}/idx00003
Content-Type: application/x-ndjson

{
  "mappings": {
    "properties": {
      "timestamp": {
        "type":   "date"
      }
    }
  }
}

### Index data failed
POST {{baseUrl}}/idx00003/_doc
Content-Type: application/x-ndjson

{"timestamp": "2015-01-01 12:10:30"}

### Index data failed
{
  "error": {
    "root_cause": [
      {
        "type": "mapper_parsing_exception",
        "reason": "failed to parse field [timestamp] of type [date] in document with id 'dyoyNpYBxdmNMfRxwqjc'. Preview of field's value: '2015-01-01 12:10:30'"
      }
    ],
    "type": "mapper_parsing_exception",
    "reason": "failed to parse field [timestamp] of type [date] in document with id 'dyoyNpYBxdmNMfRxwqjc'. Preview of field's value: '2015-01-01 12:10:30'",
    "caused_by": {
      "type": "illegal_argument_exception",
      "reason": "failed to parse date field [2015-01-01 12:10:30] with format [strict_date_optional_time||epoch_millis]",
      "caused_by": {
        "type": "date_time_parse_exception",
        "reason": "Failed to parse with all enclosed parsers"
      }
    }
  },
  "status": 400
}

Related Issues

#2489

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • 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.

penghuo added 2 commits April 14, 2025 14:22
Signed-off-by: Peng Huo <[email protected]>
Signed-off-by: Peng Huo <[email protected]>
@penghuo penghuo marked this pull request as ready for review April 15, 2025 15:46
@penghuo penghuo requested a review from qianheng-aws as a code owner April 15, 2025 15:46
penghuo added 2 commits April 15, 2025 08:47
Signed-off-by: Peng Huo <[email protected]>
Signed-off-by: Peng Huo <[email protected]>
@penghuo penghuo added the enhancement New feature or request label Apr 15, 2025
Copy link
Collaborator

@qianheng-aws qianheng-aws left a comment

Choose a reason for hiding this comment

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

Remove "2015-01-01 12:10:30" related UT, it is not valid OpenSearch default date format.

This should be a breaking change that replacing DATE_TIME_FORMATTER with DateFieldMapper.getDefaultDateTimeFormatter(), which means we no longer support parsing value with format "yyyy-MM-dd HH:mm:ss". Shall we add this change in this issue: #3248 or somewhere else for recording breaking change after 3.0 release?

And shall we keep the format align in the final results? After this change, the input value will require format with "yyyy-MM-ddTHH:mm:ss" or epoch_millis by default if no user-specified formats, while the PPL results will always show timestamp with format "yyyy-MM-dd HH:mm:ss"

@@ -262,9 +262,10 @@ private static ExprValue parseDateTimeString(String value, OpenSearchDateType da
DateFormatters.from(STRICT_YEAR_MONTH_DAY_FORMATTER.parse(value)).toLocalDate());
default:
return new ExprTimestampValue(
DateFormatters.from(DATE_TIME_FORMATTER.parse(value)).toInstant());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shall we remove DATE_TIME_FORMATTER from DateTimeFormatters since it's no longer used anywhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done. 801ca25

@penghuo
Copy link
Collaborator Author

penghuo commented Apr 16, 2025

This should be a breaking change that replacing DATE_TIME_FORMATTER with DateFieldMapper.getDefaultDateTimeFormatter(), which means we no longer support parsing value with format "yyyy-MM-dd HH:mm:ss". Shall we add this change in this issue: #3248 or somewhere else for recording breaking change after 3.0 release?

It is not a breaking change. yyyy-MM-dd HH:mm:ss it is not valid OpenSearch default date format. I explained in PR description.

And shall we keep the format align in the final results? After this change, the input value will require format with "yyyy-MM-ddTHH:mm:ss" or epoch_millis by default if no user-specified formats, while the PPL results will always show timestamp with format "yyyy-MM-dd HH:mm:ss"

Not necessary, index mapping default format enforce on ingestion. PPL could have it own default format when present result.

@qianheng-aws
Copy link
Collaborator

qianheng-aws commented Apr 16, 2025

It is not a breaking change. yyyy-MM-dd HH:mm:ss it is not valid OpenSearch default date format. I explained in PR description.

Got it, although we have such test cases before, it seems could never be happened in fact for customers because opensearch will throw error when putting such a value without customized format

qianheng-aws
qianheng-aws previously approved these changes Apr 16, 2025
Signed-off-by: Peng Huo <[email protected]>
Copy link
Collaborator

@dai-chen dai-chen left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

Any guidance when should we add test to this new yaml test? Any bug fix?

@qianheng-aws qianheng-aws merged commit 8134c4e into opensearch-project:main Apr 22, 2025
22 checks passed
@penghuo penghuo deleted the issue2489 branch April 22, 2025 17:44
@penghuo penghuo mentioned this pull request Apr 23, 2025
7 tasks
@LantaoJin LantaoJin added bug Something isn't working backport 3.0 labels Apr 25, 2025
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 25, 2025
* Fix issue 2489

Signed-off-by: Peng Huo <[email protected]>

* Update comments

Signed-off-by: Peng Huo <[email protected]>

* Integrate with YamlRestTest

Signed-off-by: Peng Huo <[email protected]>

* Revert change

Signed-off-by: Peng Huo <[email protected]>

* Fix YamlTest

Signed-off-by: Peng Huo <[email protected]>

* Disable security.manager

Signed-off-by: Peng Huo <[email protected]>

* Remove unused code

Signed-off-by: Peng Huo <[email protected]>

---------

Signed-off-by: Peng Huo <[email protected]>
(cherry picked from commit 8134c4e)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
LantaoJin pushed a commit that referenced this pull request Apr 25, 2025
* Fix issue 2489



* Update comments



* Integrate with YamlRestTest



* Revert change



* Fix YamlTest



* Disable security.manager



* Remove unused code



---------


(cherry picked from commit 8134c4e)

Signed-off-by: Peng Huo <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
penghuo added a commit that referenced this pull request Jun 16, 2025
* Fix issue 2489

Signed-off-by: Peng Huo <[email protected]>

* Update comments

Signed-off-by: Peng Huo <[email protected]>

* Integrate with YamlRestTest

Signed-off-by: Peng Huo <[email protected]>

* Revert change

Signed-off-by: Peng Huo <[email protected]>

* Fix YamlTest

Signed-off-by: Peng Huo <[email protected]>

* Disable security.manager

Signed-off-by: Peng Huo <[email protected]>

* Remove unused code

Signed-off-by: Peng Huo <[email protected]>

---------

Signed-off-by: Peng Huo <[email protected]>
Signed-off-by: xinyual <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 3.0 bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants