[BUG] Calling getMillis() in Painless script is slow

[mbaranczak]

Describe the bug

We had a sorting script that called getMillis() on a date field. We noticed that the script was unusually slow for queries that matched a large number of results, and determined that getMillis() was the bottleneck. Removing that call resulted in a 10-fold speed-up.

I found this bug report for ElasticSearch 6.5.0, which looks very similar to what I ran into. It says that the bug was fixed; maybe the fix never made it into OpenSearch, maybe the bug came back. elastic/elasticsearch#35754

I had good results with the work-around suggested in that report (replacing getMillis() with toInstant().toEpochMilli()).

To Reproduce

On an index with a datetime field called originalDate, and a large number of documents (a million or more), run this search:

{
  "_source": ["id","originalDate"],
  "sort": {
    "_script": {
      "type": "number",
      "order": "asc",
      "script": {
        "source": "return doc[\"originalDate\"].value.getMillis();"
	  }
	}
  }
}

Host/Environment:

Amazon OpenSearch service v1.0

[dblock]

Would love a PR, @mbaranczak, thank you!

[reta]

@mbaranczak interesting, here is the getMillis() implementation, I believe this slowness comes from the logDeprecatedMethod calls?

    @Deprecated
    public long getMillis() {
        logDeprecatedMethod("getMillis()", "toInstant().toEpochMilli()");
        return dt.toInstant().toEpochMilli();
    }

@dblock this method is deprecated and should not be used in general, should such deprecated methods in 2.0?

[mbaranczak]

Is it really necessary to deprecate/remove this method? If getMillis() is just an alias for toInstanct().toEpochMilli(), what's the harm in keeping it?

And here's another thing: since I'm using OpenSearch in a managed environment, I don't have access to the server log (or wherever this deprecation warning gets written to). So I don't even see the warning. In my use case, logDeprecatedMethod does nothing useful, and just wastes CPU time. Would it make sense to add a field to the response for deprecation warnings (and possibly other warnings)?

[reta]

Is it really necessary to deprecate/remove this method? If getMillis() is just an alias for toInstanct().toEpochMilli(), what's the harm in keeping it?

@mbaranczak the deprecation story goes back to Elasticsearch, this is what Opensearch inherited.

Would it make sense to add a field to the response for deprecation warnings (and possibly other warnings)?

I believe you should get them inside Warning response header: https://github.com/elastic/elasticsearch/pull/55941/files

[mbaranczak]

@reta I'm not seeing any warnings in the headers, either. Are the warning headers always on, or is there some switch to turn them on and off?

[reta]

@mbaranczak sorry, my bad, it is OFF by default but could be configured by adding

appender.header_warning.type = HeaderWarningAppender
appender.header_warning.name = header_warning                                                

to config/log4j2.properties. Is it something you could pull off? Here is the example of the Warning header returned:

Warning: 299 OpenSearch-3.0.0-SNAPSHOT-10bff0c9f5b9ca78b3dc50f5c704dabf41b9d535 "Use of the joda time method [getMillis()] is deprecated. Use [toInstant().toEpochMilli()] instead."

[dblock]

Is it really necessary to deprecate/remove this method? If getMillis() is just an alias for toInstanct().toEpochMilli(), what's the harm in keeping it?

I don't know why it was deprecated in Elasticsearch. I suppose nobody wants to keep methods that don't actually do anything other than call other methods that do the same thing around. Major versions have other breaking changes, and I think the right thing to do here would be to remove the deprecated method from 3.0 (main). This way it will break hard in a major upgrade instead of causing a silent performance regression. Want to PR it if you agree?

[reta]

@dblock 👍 to that, will create an issue + change

[mbaranczak]

@reta I'm not running my own server, I'm using the Amazon OpenSearch Service. No, I don't see any way to change the log4j properties.