Skip to content

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 <penghuo@gmail.com>
Signed-off-by: Peng Huo <penghuo@gmail.com>
Signed-off-by: Peng Huo <penghuo@gmail.com>
@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 <penghuo@gmail.com>
Signed-off-by: Peng Huo <penghuo@gmail.com>
@penghuo penghuo added the enhancement New feature or request label Apr 15, 2025
Signed-off-by: Peng Huo <penghuo@gmail.com>
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"

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 <penghuo@gmail.com>
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 <penghuo@gmail.com>

* Update comments

Signed-off-by: Peng Huo <penghuo@gmail.com>

* Integrate with YamlRestTest

Signed-off-by: Peng Huo <penghuo@gmail.com>

* Revert change

Signed-off-by: Peng Huo <penghuo@gmail.com>

* Fix YamlTest

Signed-off-by: Peng Huo <penghuo@gmail.com>

* Disable security.manager

Signed-off-by: Peng Huo <penghuo@gmail.com>

* Remove unused code

Signed-off-by: Peng Huo <penghuo@gmail.com>

---------

Signed-off-by: Peng Huo <penghuo@gmail.com>
(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 <penghuo@gmail.com>
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 <penghuo@gmail.com>

* Update comments

Signed-off-by: Peng Huo <penghuo@gmail.com>

* Integrate with YamlRestTest

Signed-off-by: Peng Huo <penghuo@gmail.com>

* Revert change

Signed-off-by: Peng Huo <penghuo@gmail.com>

* Fix YamlTest

Signed-off-by: Peng Huo <penghuo@gmail.com>

* Disable security.manager

Signed-off-by: Peng Huo <penghuo@gmail.com>

* Remove unused code

Signed-off-by: Peng Huo <penghuo@gmail.com>

---------

Signed-off-by: Peng Huo <penghuo@gmail.com>
Signed-off-by: xinyual <xinyual@amazon.com>
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