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

Map BETWEEN to (min <= value AND value <= max) #877

Merged
merged 3 commits into from Jun 5, 2019

Conversation

4 participants
@dain
Copy link
Member

commented Jun 1, 2019

No description provided.

@cla-bot cla-bot bot added the cla-signed label Jun 1, 2019

@dain dain requested a review from martint Jun 1, 2019

@dain dain changed the title Map BETWEEN to (min <= value AND value <= max) [WIP] Map BETWEEN to (min <= value AND value <= max) Jun 1, 2019

@dain dain added the WIP label Jun 1, 2019

@dain dain removed the request for review from martint Jun 1, 2019

@dain dain force-pushed the dain:between branch from 6b5db7b to 34642c9 Jun 1, 2019

@martint martint self-requested a review Jun 1, 2019

@dain dain force-pushed the dain:between branch from 34642c9 to ed190d6 Jun 2, 2019

@dain dain removed the WIP label Jun 2, 2019

@dain dain changed the title [WIP] Map BETWEEN to (min <= value AND value <= max) Map BETWEEN to (min <= value AND value <= max) Jun 2, 2019

@sopel39

sopel39 approved these changes Jun 3, 2019

return fieldReferenceCompiler.visitVariableReference(reference, context.getScope());
}
}

private static final String TEMP_PREFIX = "$$TEMP$$";

public static VariableReferenceExpression createTempVariableReferenceExpression(Variable variable, Type type)

This comment has been minimized.

Copy link
@sopel39

sopel39 Jun 3, 2019

Member

was it never needed before?

This comment has been minimized.

Copy link
@dain

dain Jun 3, 2019

Author Member

No this is only needed because we are creating a temp variable we want to refer to in another expression.

@sopel39

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

Please update the docs too

@dain dain force-pushed the dain:between branch from ed190d6 to 1d422fb Jun 4, 2019

@martint

martint approved these changes Jun 4, 2019

Copy link
Member

left a comment

Looks good other than some minor comments.

return false;
}
SpecialForm other = (SpecialForm) obj;
return this.form == other.form &&

This comment has been minimized.

Copy link
@martint

martint Jun 4, 2019

Member

Include returnType

}

if (greaterOrEqualToMin == null) {
return lessThanOrEqualToMax == Boolean.FALSE ? false : null;

This comment has been minimized.

Copy link
@martint

martint Jun 4, 2019

Member

Compare with .equals(). There's no guarantee the boolean returned by invokeOperator(...) above is Boolean.FALSE or Boolean.TRUE (they could be new Boolean(false) or new Boolean(true))

Boolean lessThanOrEqualToMax = max == null ? null : lessThanOrEquals.test(value, max);

if (greaterOrEqualToMin == null) {
return lessThanOrEqualToMax == Boolean.FALSE ? false : null;

This comment has been minimized.

Copy link
@martint

martint Jun 4, 2019

Member

Compare with .equals()

@dain dain force-pushed the dain:between branch from 1d422fb to 95e8ddd Jun 5, 2019

@dain dain force-pushed the dain:between branch from 95e8ddd to cfef9ab Jun 5, 2019

@dain dain merged commit cfef9ab into prestosql:master Jun 5, 2019

1 of 3 checks passed

Travis CI - Branch Build Created
Details
Travis CI - Pull Request Build Created
Details
verification/cla-signed
Details

@dain dain deleted the dain:between branch Jun 5, 2019

@dain dain referenced this pull request Jun 5, 2019

Closed

Release notes for 314 #879

2 of 6 tasks complete

@findepi findepi added this to the 314 milestone Jun 6, 2019

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