Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "REVERSE" / "REPEAT" / "RIGHT" / "LEFT" functions #7334

Merged
merged 6 commits into from Apr 10, 2019

Conversation

@asdf2014
Copy link
Member

commented Mar 23, 2019

Description

Add several string-related functions in this PR.

Design

  • Add left / right / repeat / reverse Druid expression functions
  • Add LEFT / RIGHT / REPEAT / REVERSE Druid SQL functions

@asdf2014 asdf2014 added the Area - SQL label Mar 23, 2019

return new String(single, StandardCharsets.UTF_8);
}
if (Integer.MAX_VALUE / count < len) {
throw new OutOfMemoryError();

This comment has been minimized.

Copy link
@egor-ryashin

egor-ryashin Mar 23, 2019

Contributor

I believe "there is a strong convention that errors are reserved for use by the JVM to indicate resource deficiencies, invariant failures, or other conditions that make it impossible to continue execution."

This comment has been minimized.

Copy link
@asdf2014

asdf2014 Mar 24, 2019

Author Member

@egor-ryashin Thanks, fixed.

@asdf2014 asdf2014 requested a review from egor-ryashin Mar 26, 2019

@@ -399,7 +399,13 @@ public static String repeat(String s, int count)
return new String(single, StandardCharsets.UTF_8);
}
if (Integer.MAX_VALUE / count < len) {
throw new OutOfMemoryError();
String errMsg = StringUtils.format(

This comment has been minimized.

Copy link
@egor-ryashin

egor-ryashin Mar 26, 2019

Contributor

I would go for a concise one The produced string is too large. I think a general Druid user is smart enough to understand it right away.

This comment has been minimized.

Copy link
@asdf2014

@asdf2014 asdf2014 requested a review from egor-ryashin Mar 27, 2019

@asdf2014

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

@egor-ryashin Do you have more comments?

don't have more comments right now

* Returns a string whose value is the concatenation of the
* string {@code str} repeated {@code count} times.
* <p>
* If count or length is zero then the empty string is returned.

This comment has been minimized.

Copy link
@QiuMM

QiuMM Apr 4, 2019

Member

nit: ... string {@code s}...

This comment has been minimized.

Copy link
@asdf2014

asdf2014 Apr 9, 2019

Author Member

@QiuMM Thanks 👍

This comment has been minimized.

Copy link
@asdf2014

asdf2014 Apr 9, 2019

Author Member

Fixed.

@QiuMM

QiuMM approved these changes Apr 8, 2019

Copy link
Member

left a comment

Overall LGTM, I left some trivial comments.

@@ -326,7 +343,6 @@ protected ExprEval eval(double param)
}
}


This comment has been minimized.

Copy link
@QiuMM

QiuMM Apr 8, 2019

Member

nit: remove this blank line.

This comment has been minimized.

Copy link
@asdf2014

asdf2014 Apr 9, 2019

Author Member

Removed.

Assert.assertEquals("", StringUtils.repeat("foo", 0));
Assert.assertEquals("foo", StringUtils.repeat("foo", 1));
Assert.assertEquals("foofoofoo", StringUtils.repeat("foo", 3));
}

This comment has been minimized.

Copy link
@QiuMM

QiuMM Apr 8, 2019

Member

nit: maybe replace } with a blank line? There is no such code style in druid. The same for the { and } below.

This comment has been minimized.

Copy link
@asdf2014

asdf2014 Apr 9, 2019

Author Member

Fixed.

@asdf2014

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

@QiuMM Thanks for your comments 👍 They have all been fixed.

@QiuMM QiuMM merged commit 2f64414 into apache:master Apr 10, 2019

2 checks passed

Inspections: pull requests (Druid) TeamCity build finished
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@asdf2014 asdf2014 deleted the asdf2014:str_functions branch Apr 10, 2019

@jihoonson jihoonson added this to the 0.15.0 milestone May 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.