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

fixes #1874 adding a substring extraction function, tests, and documentation #1986

Merged
merged 1 commit into from
Dec 3, 2015

Conversation

clintropolis
Copy link
Member

SQL style substring extraction function, which takes an index and optionally a length, returning a substring of the dimension value starting from the index of size length, or the remainder of the string if it is shorter than length or length is not supplied. If the starting index is greater than the length of the dimension value, null is returned.

{ "type" : "substring", "index" : 3, "length" : 4 }
{ "type" : "substring", "index" : 3 }

@rasahner
Copy link
Contributor

I was looking at the CACHE_TYPE_ID values and saw that TimeFormatExtractionFn.java and LookupExtractionFn.java both define it to 5.

int len = dimValue.length();

if (index < len) {
if (length > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this check required ? At this point length will be greater than 0 as we will return null if the length is 0 at line 62 Strings.isNullOrEmpty(dimValue)

Copy link
Member

Choose a reason for hiding this comment

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

sorry I confused length with len - ignore the comment

@fjy
Copy link
Contributor

fjy commented Nov 18, 2015

@clintropolis can we fix merge conflicts? I think this is almost ready to go

@clintropolis
Copy link
Member Author

Yep, will do this evening, along with other suggestions.

@drcrallen
Copy link
Contributor

io.druid.server.namespace.cache.NamespaceExtractionCacheManagerExecutorsTest
testConcurrentAddDelete(io.druid.server.namespace.cache.NamespaceExtractionCacheManagerExecutorsTest)  Time elapsed: 600.018 sec  <<< ERROR!
java.lang.Exception: test timed out after 600000 milliseconds
    at sun.misc.Unsafe.park(Native Method)
    at java.util.concurrent.locks.LockSupport.park(LockSupport.java:175)
    at java.util.concurrent.FutureTask.awaitDone(FutureTask.java:429)
    at java.util.concurrent.FutureTask.get(FutureTask.java:191)
    at 

Bouncing for travis

@drcrallen drcrallen closed this Nov 19, 2015
@drcrallen drcrallen reopened this Nov 19, 2015
@fjy fjy closed this Nov 19, 2015
@fjy fjy reopened this Nov 19, 2015
}

@Override
public byte[] getCacheKey()
Copy link
Contributor

Choose a reason for hiding this comment

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

thought @drcrallen want to have at lease one UT for getCacheKey ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please :)

@fjy
Copy link
Contributor

fjy commented Nov 20, 2015

@clintropolis can we finish this off?

@drcrallen
Copy link
Contributor

BrokerServerViewTest.testSingleServerAddedRemovedSegment:112->setupZNodeForServer:383 » NodeExists

@drcrallen drcrallen closed this Nov 20, 2015
@drcrallen drcrallen reopened this Nov 20, 2015
@clintropolis
Copy link
Member Author

@fjy I think it should be good to go now

@Override
public boolean preservesOrdering()
{
return false;
Copy link
Member

Choose a reason for hiding this comment

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

this should be true when index == 0 to allow optimizations for this particular case.

)
{

Preconditions.checkArgument(length == null || length >= 0, "length must be positive");
Copy link
Contributor

Choose a reason for hiding this comment

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

is length == 0 allowed? then it is expected to be "non-negative" not "positive"

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, from the code below it appears that length being 0 is not allowed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I probably should not allow a length of 0, since it just returns null across the board. I can change the check.

Copy link
Member

Choose a reason for hiding this comment

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

@himanshug @clintropolis "positive" means >= 0 "strictly positive" means > 0. If we don't allow 0, it should be say strictly positive to be clear.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, will change message since I don't think having a length of 0 is very useful.

@fjy
Copy link
Contributor

fjy commented Dec 2, 2015

👍 @himanshug @drcrallen i think this is ready to go

@himanshug
Copy link
Contributor

👍

@himanshug himanshug closed this Dec 3, 2015
@himanshug himanshug reopened this Dec 3, 2015
himanshug added a commit that referenced this pull request Dec 3, 2015
fixes #1874 adding a substring extraction function, tests, and documentation
@himanshug himanshug merged commit 00c6027 into apache:master Dec 3, 2015
@fjy fjy modified the milestone: 0.9.0 Feb 4, 2016
@fjy fjy mentioned this pull request Feb 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants