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

Case functions support: LOWER, UPPER #177

Merged
merged 13 commits into from
Sep 5, 2019
Merged

Case functions support: LOWER, UPPER #177

merged 13 commits into from
Sep 5, 2019

Conversation

galkk
Copy link
Contributor

@galkk galkk commented Sep 5, 2019

Issue #128

Description of changes:

  • Added 2 new functions LOWER and UPPER that receive field name as a first parameter and, potentially, locale

  • Changed identifier generation strategy to id per function name instead of global id

While I feel that SQLFunctions should be refactored, to expedite the process as of now I'm adding just functions implemented by analogy with trim() and substring() functions

Testing

integration testing, testing via kibana

Itegration tests include ORDER BY and GROUP BY clauses to ensure that everything is working.
Found 2 new issues that will be addressed separately: #176 , #175

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

"WHERE LOWER(e.lastname)='duke' " +
"GROUP BY UPPER(e.firstname) ";

System.out.println(executeQuery(query));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this.

Copy link
Contributor Author

@galkk galkk Sep 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, I feel that we need codestyle rule for those slips :)

* Generates next id for given method name. The id's are increasing for each method name, so
* nextId("a"), nextId("a"), nextId("b") will return a_1, a_2, b_1
*/
public String nextId(String methodName) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think currently, the methodName is cached in request level. If i am right, does this cached method could be shared by different request?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. The SqlFunctions is per-request, and this is instance field

Copy link
Contributor

@penghuo penghuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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.

Looks good! Thanks!

@galkk galkk merged commit e33ec1b into opendistro-for-elasticsearch:master Sep 5, 2019
@galkk galkk deleted the case-functions-support branch September 5, 2019 23:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants