Skip to content
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

Support Alternate Datetime Formats #1664

Merged

Conversation

GumpacG
Copy link
Collaborator

@GumpacG GumpacG commented May 29, 2023

Description

Supports using date/time/timestamp formats obtained from OpenSearch. Contains some changes from OpenSearchDataType rework (Bit-Quill@8b2f65d). Resolves some issues previously seen on the forum (please see Issues Resolved section below).

Formats that require only dates will return type of date. For example, the format "basic_date" will have type date in the result. Formats that require only times will return type of time. For example, the format "basic_time" will have type time in the result. Formats that can have either date or time will return type of timestamp. For example, the format "basic_date||basic_time" will have type timestamp in the result. This is also the case when using aggregation like AVG.

Out of scope

Custom formats in the index mapping will be supported in a follow-up issue

TODO: Follow up with support for user-defined formats

Based on PoC (Here: https://github.com/Bit-Quill/opensearch-project-sql/pull/169/files).

Note: With these changes, issues may be introduced where data is read differently if a user's mapping defines a format. This is because the plugin previously used a hardcoded formatter to parse the data, while with these changes it will use the formatter defined in the mapping.

Supported formats include named formats listed in: opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDateType.java or https://opensearch.org/docs/latest/field-types/supported-field-types/date/

Issues Resolved

#794
https://forum.opensearch.org/t/sql-select-fails-on-date-fields-format-epoch-second/11521

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • 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.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@codecov
Copy link

codecov bot commented May 29, 2023

Codecov Report

Merging #1664 (1591a85) into main (9824b1c) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #1664      +/-   ##
============================================
+ Coverage     97.16%   97.19%   +0.02%     
- Complexity     4120     4165      +45     
============================================
  Files           371      372       +1     
  Lines         10373    10478     +105     
  Branches        704      720      +16     
============================================
+ Hits          10079    10184     +105     
  Misses          287      287              
  Partials          7        7              
Flag Coverage Δ
sql-engine 97.19% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...h/sql/opensearch/data/type/OpenSearchDataType.java 100.00% <100.00%> (ø)
...h/sql/opensearch/data/type/OpenSearchDateType.java 100.00% <100.00%> (ø)
...h/sql/opensearch/data/type/OpenSearchTextType.java 100.00% <100.00%> (ø)
...nsearch/data/value/OpenSearchExprValueFactory.java 100.00% <100.00%> (ø)
...pensearch/sql/opensearch/mapping/IndexMapping.java 100.00% <100.00%> (ø)

* Add OpenSearchDateType as a datatype for matching with Date/Time OpenSearch types

Signed-off-by: Andrew Carbonetto <andrewc@bitquilltech.com>

* Added IT Test

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Added Basic Implementation

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Added A Comment

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Added Support For Default OpenSearch Formats

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Code Cleanup

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Refactored 'parsetimestamp' Function

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Cleanup

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Removed Unnecessary Try-Catch Block

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Fixed Unit Tests

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Fixed IT Test

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Fixed Text Type and Disabled JSON Format Test

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Fix Mapping Type Reporting As Date Instead Of Timestamp In Schema

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Revert Breaking Test Changes

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Fixed Returned ExprCoreTypes

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Fixed OpenSearch Type Names In Mapping

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Fixed LegacyTypeName Function And Refactored Code To Fix Calculations With Datetime Types

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Changed Support For Passing Fields Into Text Type

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Reverted Some Changes And Commented Out A Test

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Commented Out A Breaking Change

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Minor Refactoring Changes

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Reworked parseTimestamp Function To Format Return Based On Enum

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Fixed Returned ExprValue

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Reverted Changes To Test Files

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Reverted Some Changes To Enum And Reworked Code

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Added Type Mapping To Allow Query To Be Parsed

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Fixed Some Unit Tests

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Fixed TypeActionMap To Still Support ExprCoreTypes

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Wrapped Some Function Returns in ExprValueFactory

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Fixed Integration Test Bug

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
(cherry picked from commit 2c16a554573015fcd752721353b2713858caad1a)

* Slight Refactoring And Code Cleanup

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
(cherry picked from commit ebecec79c79d75e851a4a0914cf4abf5aa6d3efd)

* Removed Unneeded Class Member

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
(cherry picked from commit 14de39815ebeecd36c4155099d875a45a7e008f6)

* Removed Unneccessary MappingType Parameters

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
(cherry picked from commit e6a8bc3a6a3d7a11dacd03fc885871f3b055d2d2)

* Refactored Code To Fix Integration Test

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
(cherry picked from commit c91f48d9763499940ecf302212fcd592aecb6018)

* Addressed JaCoCo Issues

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Fixed Checkstyle

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Changed gradle version and removed values iterator

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Addressed PR Comments

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Update a test to match new indexResponse.aliases() type.

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

* Changed To Throw Exception When Encountering Parsing Failures

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Fixed Checkstyle

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Removed Unneeded Imports

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Re-enabled Some Tests

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Addressed PR Comments

Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>

* Changed test data

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Pulled out support for user-defined datetime formats

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Modified IT test data

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Added IT tests

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Removed support for just time and just date formats

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Removed unsupported formats from IT

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Removed unit tests for date only or time only formats

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Addressed PR comments from previous PR

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Addressed PR comment regarding unnecessary OpenSearchDateType instance in OpenSearchDataType

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Updates to include build-in date and time formats for OpenSearchDateTypes

Signed-off-by: Andrew Carbonetto <andrewc@bitquilltech.com>

* Add tests and fix checkstyle

Signed-off-by: Andrew Carbonetto <andrewc@bitquilltech.com>

* Fixed bugs regarding custom formats

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Add unit test to cover custom formats

Signed-off-by: Andrew Carbonetto <andrewc@bitquilltech.com>

* Addressed PR comments

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Fixed checkstyle errors

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Modified integration test

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Fixed Date and time aggregation

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Fixed failing integ tests

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Modified integ test based on date format changes

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Added default formatters for date and time. Added unit tests for coverage

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Some minor review comments

Signed-off-by: Andrew Carbonetto <andrewc@bitquilltech.com>

* Addressed PR comments

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Fixed checktsyle and jacoco

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Changed link in rst file

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Changed link in rst file

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Addressed PR comments

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

---------

Signed-off-by: Andrew Carbonetto <andrewc@bitquilltech.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
Co-authored-by: Andrew Carbonetto <andrewc@bitquilltech.com>
Co-authored-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Co-authored-by: MaxKsyunz <maxk@bitquilltech.com>
@GumpacG GumpacG force-pushed the integ-support-datetime-formats branch from 0e5ec71 to 1345794 Compare May 30, 2023 17:00
Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>
@Yury-Fridlyand Yury-Fridlyand mentioned this pull request May 30, 2023
6 tasks
@acarbonetto acarbonetto merged commit 986db39 into opensearch-project:main May 30, 2023
@acarbonetto acarbonetto deleted the integ-support-datetime-formats branch May 30, 2023 20:16
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 30, 2023
* Support Alternate Datetime Formats (#268)

* Add OpenSearchDateType as a datatype for matching with Date/Time OpenSearch types

Signed-off-by: Andrew Carbonetto <andrewc@bitquilltech.com>

---------

Signed-off-by: Andrew Carbonetto <andrewc@bitquilltech.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
Co-authored-by: Andrew Carbonetto <andrewc@bitquilltech.com>
Co-authored-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Co-authored-by: MaxKsyunz <maxk@bitquilltech.com>
(cherry picked from commit 986db39)
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 30, 2023
* Support Alternate Datetime Formats (#268)

* Add OpenSearchDateType as a datatype for matching with Date/Time OpenSearch types

Signed-off-by: Andrew Carbonetto <andrewc@bitquilltech.com>

---------

Signed-off-by: Andrew Carbonetto <andrewc@bitquilltech.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
Co-authored-by: Andrew Carbonetto <andrewc@bitquilltech.com>
Co-authored-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Co-authored-by: MaxKsyunz <maxk@bitquilltech.com>
(cherry picked from commit 986db39)
acarbonetto pushed a commit that referenced this pull request May 30, 2023
* Support Alternate Datetime Formats (#268)

* Add OpenSearchDateType as a datatype for matching with Date/Time OpenSearch types

Signed-off-by: Andrew Carbonetto <andrewc@bitquilltech.com>

---------

Signed-off-by: Andrew Carbonetto <andrewc@bitquilltech.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
Co-authored-by: Andrew Carbonetto <andrewc@bitquilltech.com>
Co-authored-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Co-authored-by: MaxKsyunz <maxk@bitquilltech.com>
(cherry picked from commit 986db39)

Co-authored-by: Guian Gumpac <guian.gumpac@improving.com>
acarbonetto pushed a commit that referenced this pull request May 30, 2023
* Support Alternate Datetime Formats (#268)

* Add OpenSearchDateType as a datatype for matching with Date/Time OpenSearch types

Signed-off-by: Andrew Carbonetto <andrewc@bitquilltech.com>

---------

Signed-off-by: Andrew Carbonetto <andrewc@bitquilltech.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
Co-authored-by: Andrew Carbonetto <andrewc@bitquilltech.com>
Co-authored-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Co-authored-by: MaxKsyunz <maxk@bitquilltech.com>
(cherry picked from commit 986db39)

Co-authored-by: Guian Gumpac <guian.gumpac@improving.com>
MitchellGale pushed a commit to Bit-Quill/opensearch-project-sql that referenced this pull request Jun 12, 2023
* Support Alternate Datetime Formats (#268)

* Add OpenSearchDateType as a datatype for matching with Date/Time OpenSearch types

Signed-off-by: Andrew Carbonetto <andrewc@bitquilltech.com>

---------

Signed-off-by: Andrew Carbonetto <andrewc@bitquilltech.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
Co-authored-by: Andrew Carbonetto <andrewc@bitquilltech.com>
Co-authored-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Co-authored-by: MaxKsyunz <maxk@bitquilltech.com>
MitchellGale pushed a commit to Bit-Quill/opensearch-project-sql that referenced this pull request Jun 12, 2023
* Support Alternate Datetime Formats (#268)

* Add OpenSearchDateType as a datatype for matching with Date/Time OpenSearch types

Signed-off-by: Andrew Carbonetto <andrewc@bitquilltech.com>

---------

Signed-off-by: Andrew Carbonetto <andrewc@bitquilltech.com>
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
Co-authored-by: Andrew Carbonetto <andrewc@bitquilltech.com>
Co-authored-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
Co-authored-by: MaxKsyunz <maxk@bitquilltech.com>
Signed-off-by: Mitchell Gale <Mitchell.Gale@improving.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants