Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Bug fix, using Local.Root when format the string #767

Conversation

penghuo
Copy link
Contributor

@penghuo penghuo commented Oct 2, 2020

Description of changes:

The DateTimeFunctionIT.testWeek failed when running with locale=th-TH-u-nu-thai-x-lvariant-TH

REPRODUCE WITH: ./gradlew ':integ-test:integTestWithNewEngineRunner' --tests "com.amazon.opendistroforelasticsearch.sql.ppl.DateTimeFunctionIT.testWeek" -Dtests.seed=533E63C3378AF235 -Dtests.security.manager=false -Dtests.locale=th-TH-u-nu-thai-x-lvariant-TH -Dtests.timezone=Pacific/Tongatapu -Druntime.java=14

com.amazon.opendistroforelasticsearch.sql.ppl.DateTimeFunctionIT > testWeek FAILED
    org.elasticsearch.client.ResponseException: method [POST], host [http://127.0.0.1:49687], URI [/_opendistro/_ppl], status line [HTTP/1.1 400 Bad Request]
    {
      "error": {
        "reason": "Invalid Query",
        "details": "Failed to parse query due to offending symbol [)] at: 'source=elasticsearch-sql_test_index_date | eval f = week(date('2008-02-20'), ๐)' <--- HERE... More details: Expecting tokens in {'D', 'NOT', 'TRUE', 'FALSE', 'INTERVAL', 'MICROSECOND', 'SECOND', 'MINUTE', 'HOUR', 'DAY', 'WEEK', 'MONTH', 'QUARTER', 'YEAR', '.', '+', '-', '(', '`', 'AVG', 'COUNT', 'MAX', 'MIN', 'SUM', 'ABS', 'CEIL', 'CEILING', 'CONV', 'CRC32', 'E', 'EXP', 'FLOOR', 'LN', 'LOG', 'LOG10', 'LOG2', 'MOD', 'PI', 'POW', 'POWER', 'RAND', 'ROUND', 'SIGN', 'SQRT', 'TRUNCATE', 'ACOS', 'ASIN', 'ATAN', 'ATAN2', 'COS', 'COT', 'DEGREES', 'RADIANS', 'SIN', 'TAN', 'ADDDATE', 'DATE', 'DATE_ADD', 'DATE_SUB', 'DAYOFMONTH', 'DAYOFWEEK', 'DAYOFYEAR', 'DAYNAME', 'FROM_DAYS', 'MONTHNAME', 'SUBDATE', 'TIME', 'TIME_TO_SEC', 'TIMESTAMP', 'DATE_FORMAT', 'TO_DAYS', 'SUBSTR', 'SUBSTRING', 'LTRIM', 'RTRIM', 'TRIM', 'LOWER', 'UPPER', 'CONCAT', 'CONCAT_WS', 'LENGTH', 'STRCMP', ID, INTEGER_LITERAL, DECIMAL_LITERAL, DQUOTA_STRING, SQUOTA_STRING, BQUOTA_STRING}",
        "type": "SyntaxCheckException"
      },
      "status": 400
    }
        at __randomizedtesting.SeedInfo.seed([533E63C3378AF235:E964A9F36D151098]:0)
        at org.elasticsearch.client.RestClient.convertResponse(RestClient.java:302)
        at org.elasticsearch.client.RestClient.performRequest(RestClient.java:272)
        at org.elasticsearch.client.RestClient.performRequest(RestClient.java:246)
        at com.amazon.opendistroforelasticsearch.sql.ppl.PPLIntegTestCase.executeQueryToString(PPLIntegTestCase.java:42)
        at com.amazon.opendistroforelasticsearch.sql.ppl.PPLIntegTestCase.executeQuery(PPLIntegTestCase.java:38)
        at com.amazon.opendistroforelasticsearch.sql.ppl.DateTimeFunctionIT.week(DateTimeFunctionIT.java:374)
        at com.amazon.opendistroforelasticsearch.sql.ppl.DateTimeFunctionIT.testWeek(DateTimeFunctionIT.java:387)

The following test code explain why it failed and how to handle this cases.

    @Test
    public void demo_locale() {
        Locale.Builder builder = new Locale.Builder();
        builder.setLanguageTag("th-TH-u-nu-thai-x-lvariant-TH");
        final Locale localeThai = builder.build();
        assertEquals("week(date('2008-02-20'), ๐)", String.format(localeThai, "week(date('2008-02-20'), %d)", 0));
        assertEquals("week(date('2008-02-20'), 0)", String.format(Locale.ROOT, "week(date('2008-02-20'), %d)", 0));
    }

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@penghuo penghuo requested review from dai-chen and chloe-zh October 2, 2020 23:53
@penghuo penghuo self-assigned this Oct 5, 2020
Copy link
Member

@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 fix!

@penghuo penghuo merged commit 50715ad into opendistro-for-elasticsearch:develop Oct 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants