Skip to content

ESQL: Add TO_UPPER and TO_LOWER functions#104309

Merged
luigidellaquila merged 6 commits intoelastic:mainfrom
luigidellaquila:esql/to_upper_lower
Jan 15, 2024
Merged

ESQL: Add TO_UPPER and TO_LOWER functions#104309
luigidellaquila merged 6 commits intoelastic:mainfrom
luigidellaquila:esql/to_upper_lower

Conversation

@luigidellaquila
Copy link
Copy Markdown
Contributor

Adding two new string funcitons

  • TO_UPPER(value): returns the input string converted to upper case
  • TO_LOWER(value): returns the input string converted to lower case

Related to: #103599

@github-actions
Copy link
Copy Markdown
Contributor

Documentation preview:

@elasticsearchmachine elasticsearchmachine added v8.13.0 Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Jan 12, 2024
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)

@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Hi @luigidellaquila, I've created a changelog YAML for you.

Copy link
Copy Markdown
Contributor

@bpintea bpintea left a comment

Choose a reason for hiding this comment

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

LGTM, but I think some tests with non-ascii chars would be welcome.

public ExpressionEvaluator.Factory toEvaluator(Function<Expression, ExpressionEvaluator.Factory> toEvaluator) {
var fieldEvaluator = toEvaluator.apply(field);
return new ToLowerEvaluator.Factory(source(), fieldEvaluator, ((EsqlConfiguration) configuration()).locale());
// return dvrCtx -> new ToLowerEvaluator(source(), fieldEvaluator.get(dvrCtx), ((EsqlConfiguration) configuration()).locale(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

leftover?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's a leftover. Fixing

some tests with non-ascii chars would be welcome.

Adding some

Copy link
Copy Markdown
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

Left some minor comments - otherwise LGTM

import static org.elasticsearch.xpack.ql.expression.TypeResolutions.ParamOrdinal.DEFAULT;
import static org.elasticsearch.xpack.ql.expression.TypeResolutions.isString;

public class ToLower extends ConfigurationFunction implements EvaluatorMapper {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since this is a unary expression, add a replaceChild() method plus field() method to both classes.

Comment on lines +1462 to +1464
List<Expression> fields = toLower.children();
assert fields.size() == 1;
out.writeExpression(fields.get(0));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use toLower/toUpper.field() here instead and drop the assert.


@Override
protected Expression build(Source source, List<Expression> args) {
return new ToLower(source, args.get(0), EsqlTestUtils.configuration(""));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

EsqlTestUtils.TEST_CFG


import static org.hamcrest.Matchers.equalTo;

public class ToLowerTests extends AbstractFunctionTestCase {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add some test with random locale as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.13.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants