[WIP] Use mapping format for reading date values

[Yury-Fridlyand]

Signed-off-by: Yury-Fridlyand [email protected]

Description

The fix is to collect formats from mapping and pass it to OpenSearchExprValueFactory.
I added mapping2 member to IndexMapping and OpenSearchExprValueFactory which hold required data. The original mapping is not removed yet to ensure that everything works as was before.

Test data

date_formats.json.txt
date_formats_mappings.json.txt

curl -s -XDELETE "http://localhost:9200/date-formats?pretty"
curl -s -H 'Content-Type: application/json' -XPUT "http://localhost:9200/date-formats?pretty" --data-binary @date_formats_mappings.json
curl -s -H 'Content-Type: application/json' -XPOST "http://localhost:9200/date-formats/_bulk?pretty" --data-binary @date_formats.json

Test queries

select * from date-formats;
select <format> from date-formats where name = '<format>';

search source=date-formats;
search source=date-formats | where name='<format>' | fields <format>;

For example, epoch_second (was reported on forum)

opensearchsql> select epoch_second from date-formats where name = 'epoch_second';
fetched rows / total rows = 1/1
+-------------------------------+
| epoch_second                  |
|-------------------------------|
| 1984-04-12 09:07:42.000123456 |
+-------------------------------+

Unsupported formats

weekyear
strict_weekyear
weekyear_year
strict_weekyear
year
strict_year
year_month
strict_year_month

(see links below)
They fall back to error:

opensearch-project-sql/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java

Lines 258 to 261 in f335101

	// TODO throw exception
	LogManager.getLogger(OpenSearchExprValueFactory.class).error(
	String.format("Can't recognize parsed value: %s, %s", parsed, parsed.getClass()));
	return new ExprStringValue(value.stringValue());

Time interpreting issue

opensearch-project-sql/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java

Line 254 in f335101

return new ExprTimestampValue(new ExprTimeValue(LocalTime.from(parsed)).timestampValue());

goes to

opensearch-project-sql/core/src/main/java/org/opensearch/sql/data/model/ExprTimeValue.java

Line 62 in f335101

return LocalDate.now();

There is no option to access queryContext there.
This could be fixed by one of

• Returning ExprTimeValue
• Applying time value on top of Epoch or any other fixed date. Note - this should be consistent across the plugin.

TBD

• What to do with LocalTime?
• What to do with unsupported formats?
• Can we return not ExprTimestampValue for date and time columns? Note: ensure that entire column has the same or similar format(s).

TODO

• See TODO comments in code
• Fix SQLCorrectnessIT with query SELECT Cancelled, AvgTicketPrice, FlightDelayMin, Carrier, timestamp FROM opensearch_dashboards_sample_data_flights. Test results - Class org.opensearch.sql.sql.SQLCorrectnessIT.pdf
  Wrong data returned for timestamp, mapping "format" : "yyyy-MM-dd HH:mm:ss"

Links

• Formats documentation: https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-date-format.html#built-in-date-formats
• Formats code: https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/common/time/DateFormatters.java

Issues Resolved

opensearch-project#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.

[MaxKsyunz]

What was this required for?

It would need to account for query start time.

[Yury-Fridlyand]

1. We return TIMESTAMP for OpenSearch date type => we return ExprTimestampValue
2. An index can store time in date field => parser returns LocalTime

To satisfy both 1 and 2 we have to build ExprTimestampValue from LocalTime which calls to LocalDate.now().
I see few ways to fix/avoid this:

1. Don't return ExprTimestampValue where it is possible. It is nice to have, but still we would have to build ExprTimestampValue from LocalTime.
2. Change how we build ExprTimestampValue from LocalTime - use a fixed date (e.g. Epoch) instead of today.
3. Deliver queryContext to OpenSearchExprValueFactory in opensearch module.

[Yury-Fridlyand]

@penghuo, @dai-chen, please have a look on this PoC. Your feedback and opinion is highly appreciated.

[dai-chen]

Because MappingEntry is only used by date time, I'm think can we add this mapping info to Content?

[dai-chen]

We may need more thoughts about this new abstraction. For now it seems only for OpenSearch date field mapping. Can we extend it to carry mapping info for all OpenSearch fields? For example, multi-field may use this abstraction to get inner field name and type.

{
  "mappings": {
    "properties": {
      "city": {
        "type": "text",
        "fields": {
          "raw": { 
            "type":  "keyword"
          }
        }
      }
    }
  }
}

[Yury-Fridlyand]

Yes, current implementation doesn't work with keyword and nested types.

[dai-chen]

I'm thinking should we refactor OpenSearchDataType? Rather than static Enum, we instantiate it with mapping info for each field during analyzing.

class OpenSearchDataType implements ExprType:

  val mappingInfo;

  val osTypeName;

  ...

[Yury-Fridlyand]

@dai-chen,
Please see changes in #180. They don't contain date format processing, but only refactor of OpenSearchDataType.

[Yury-Fridlyand]

Superseded by #273