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

Some minor styling cleanup and simplification #38

Merged
merged 1 commit into from Sep 10, 2017

Conversation

Projects
None yet
2 participants
@liancheng
Contributor

liancheng commented Sep 5, 2017

This PR makes some minor cleanup and simplifications to TiStrategy.scala to make the code more readable and idiomatic.

@@ -74,7 +65,7 @@ class TiStrategy(context: SQLContext) extends Strategy with Logging {
filters: Seq[Expression],
source: TiDBRelation,
selectRequest: TiSelectRequest = new TiSelectRequest): TiSelectRequest = {
val tiFilters = filters.map(expr => expr match { case BasicExpression(expr) => expr })

This comment has been minimized.

@liancheng

liancheng Sep 5, 2017

Contributor

A map call with a single case branch is dangerous since it may throw MatchError at runtime if this case branch doesn't cover all the cases.

@liancheng

liancheng Sep 5, 2017

Contributor

A map call with a single case branch is dangerous since it may throw MatchError at runtime if this case branch doesn't cover all the cases.

selectRequest.addAggregate(new TiFirst(arg),
fromSparkType(aggExpr.aggregateFunction.dataType))
case _ => None

This comment has been minimized.

@liancheng

liancheng Sep 5, 2017

Contributor

Returning a None in a foreach closure call looks weird since the closure returns Unit. The original None is simply discarded.

@liancheng

liancheng Sep 5, 2017

Contributor

Returning a None in a foreach closure call looks weird since the closure returns Unit. The original None is simply discarded.

@zhexuany zhexuany self-requested a review Sep 10, 2017

@zhexuany

LGTM

@zhexuany

This comment has been minimized.

Show comment
Hide comment
@zhexuany

zhexuany Sep 10, 2017

Member

Just pulled ur changes and tested it locally. merging it.

Member

zhexuany commented Sep 10, 2017

Just pulled ur changes and tested it locally. merging it.

@zhexuany zhexuany merged commit d27ff4e into pingcap:master Sep 10, 2017

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