Skip to content

Conversation

@lins05
Copy link
Contributor

@lins05 lins05 commented Feb 24, 2017

What changes were proposed in this pull request?

Fixed the line ending of FilterEstimation.scala (It's still using \n\r). Also improved the tests to cover the cases where the literals are on the left side of a binary operator.

How was this patch tested?

Existing unit tests.

@lins05
Copy link
Contributor Author

lins05 commented Feb 24, 2017

cc @ron8hu @cloud-fan

@SparkQA
Copy link

SparkQA commented Feb 24, 2017

Test build #73401 has started for PR 17051 at commit 0f56d0f.

}

}
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

please make sure the only change is line ending

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The diff shown here is because I also removed an unused import. I'll revert it to make review easier.

Copy link
Contributor Author

@lins05 lins05 Feb 24, 2017

Choose a reason for hiding this comment

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

Done in bc1d2a6 . But that seems not affecting the diffs.

@SparkQA
Copy link

SparkQA commented Feb 24, 2017

Test build #73404 has started for PR 17051 at commit 8881d58.

// AND/OR/NOT), swap the sides of the attribte and the literal, reverse the
// operator, and then check again.
val rewrittenFilter = filterNode transformExpressionsDown {
case op @ EqualTo(ar: AttributeReference, l: Literal) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: case b @ BinaryComparison(ar: AttributeReference, l: Literal) => b.withNewChildren(l, ar)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

I tried to find something like this but failed to, so I resorted to the current code. Thanks for the tip!

Copy link
Contributor Author

@lins05 lins05 Feb 24, 2017

Choose a reason for hiding this comment

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

Emm, we not only switch the side of the attr and the literal, but also reverse the operator, e.g. LessThan would be changed to GreaterThan, so we can keep the filter output not affected. So I guess we can't use withNewChildren here.

@lins05
Copy link
Contributor Author

lins05 commented Feb 24, 2017

jenkins retest this please

@SparkQA
Copy link

SparkQA commented Feb 24, 2017

Test build #73414 has finished for PR 17051 at commit 8881d58.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@asfgit asfgit closed this in 05954f3 Feb 24, 2017
Yunni pushed a commit to Yunni/spark that referenced this pull request Feb 27, 2017
## What changes were proposed in this pull request?

Fixed the line ending of `FilterEstimation.scala` (It's still using `\n\r`). Also improved the tests to cover the cases where the literals are on the left side of a binary operator.

## How was this patch tested?

Existing unit tests.

Author: Shuai Lin <[email protected]>

Closes apache#17051 from lins05/fix-cbo-filter-file-encoding.
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.

3 participants