Skip to content

Conversation

@HyukjinKwon
Copy link
Member

As I talked with Cheng,

  1. I added EquelNullSafe to ParquetFilters
    • It uses the same equality comparison filter with EqualTo since the Parquet filter performs actually null-safe equality comparison.
  2. Updated the test code (ParquetFilterSuite)
    • Convert catalyst.Expression to sources.Filter
    • Removed Cast since only Literal is picked up as a proper Filter in DataSourceStrategy
    • Added EquelNullSafe comparison
  3. Removed deprecated createFilter for catalyst.Expression

Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename maybeAnalyzedPredicate to analyzedPredicates since it's now a Seq[Expression]. Prefix maybe is for Option[T] by convention.

@liancheng
Copy link
Contributor

ok to test

@SparkQA
Copy link

SparkQA commented Aug 19, 2015

Test build #41185 has finished for PR 8275 at commit e5f556b.

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

@SparkQA
Copy link

SparkQA commented Aug 19, 2015

Test build #41204 has finished for PR 8275 at commit 208411a.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class StringIndexerModel (
    • class ElementwiseProduct(JavaTransformer, HasInputCol, HasOutputCol):
    • implicit class StringToColumn(val sc: StringContext)
    • case class FilterNode(condition: Expression, child: LocalNode) extends UnaryLocalNode
    • abstract class LocalNode extends TreeNode[LocalNode]
    • abstract class LeafLocalNode extends LocalNode
    • abstract class UnaryLocalNode extends LocalNode
    • case class ProjectNode(projectList: Seq[NamedExpression], child: LocalNode) extends UnaryLocalNode
    • case class SeqScanNode(output: Seq[Attribute], data: Seq[InternalRow]) extends LeafLocalNode
    • public final class UTF8String implements Comparable<UTF8String>, Externalizable

@HyukjinKwon
Copy link
Member Author

Could you merge this :) ?

@liancheng
Copy link
Contributor

Thanks! Merging to master (not to branch-1.5 at this moment since this is a new feature).

@asfgit asfgit closed this in ba5f7e1 Aug 20, 2015
asfgit pushed a commit that referenced this pull request Aug 28, 2015
…lter.

As I talked with Lian,

1. I added EquelNullSafe to ParquetFilters
 - It uses the same equality comparison filter with EqualTo since the Parquet filter performs actually null-safe equality comparison.

2. Updated the test code (ParquetFilterSuite)
 - Convert catalyst.Expression to sources.Filter
 - Removed Cast since only Literal is picked up as a proper Filter in DataSourceStrategy
 - Added EquelNullSafe comparison

3. Removed deprecated createFilter for catalyst.Expression

Author: hyukjinkwon <[email protected]>
Author: 권혁진 <[email protected]>

Closes #8275 from HyukjinKwon/master.

(cherry picked from commit ba5f7e1)
Signed-off-by: Cheng Lian <[email protected]>
@liancheng
Copy link
Contributor

I backported this PR to branch-1.5 to fix 1.5 Jenkins build failures. Didn't merge this PR to branch-1.5 before because it introduced new feature after branch-1.5 being cut. This PR also refactored ParquetFilterSuite and cleaned up outdated code path used by the old Parquet support which has already been removed. The problem is that, PR #8403 merged later depends on the test suite refactoring to pass the build. That's why #8403 passes for master but breaks branch-1.5.

This backporting should be safe, since the EqualSafeNull support has already been merged into branch-1.5 by #8403 (probably done by Git automatically and unintentionally). So this backporting only introduces test suite changes. See changed files of this backporting commit.

cc @pwendell @rxin @JoshRosen

asfgit pushed a commit that referenced this pull request Nov 9, 2015
…ilters code

Actually this was resolved by #8275.

But I found the JIRA issue for this is not marked as resolved since the PR above was made for another issue but the PR above resolved both.

I commented that this is resolved by the PR above; however, I opened this PR as I would like to just add
a little bit of corrections.

In the previous PR, I refactored the test by not reducing just collecting filters; however, this would not test  properly `And` filter (which is not given to the tests). I unintentionally changed this from the original way (before being refactored).

In this PR, I just followed the original way to collect filters by reducing.

I would like to close this if this PR is inappropriate and somebody would like this deal with it in the separate PR related with this.

Author: hyukjinkwon <[email protected]>

Closes #9554 from HyukjinKwon/SPARK-9557.

(cherry picked from commit 9565c24)
Signed-off-by: Michael Armbrust <[email protected]>
asfgit pushed a commit that referenced this pull request Nov 9, 2015
…ilters code

Actually this was resolved by #8275.

But I found the JIRA issue for this is not marked as resolved since the PR above was made for another issue but the PR above resolved both.

I commented that this is resolved by the PR above; however, I opened this PR as I would like to just add
a little bit of corrections.

In the previous PR, I refactored the test by not reducing just collecting filters; however, this would not test  properly `And` filter (which is not given to the tests). I unintentionally changed this from the original way (before being refactored).

In this PR, I just followed the original way to collect filters by reducing.

I would like to close this if this PR is inappropriate and somebody would like this deal with it in the separate PR related with this.

Author: hyukjinkwon <[email protected]>

Closes #9554 from HyukjinKwon/SPARK-9557.
kiszk pushed a commit to kiszk/spark-gpu that referenced this pull request Dec 26, 2015
…ilters code

Actually this was resolved by apache/spark#8275.

But I found the JIRA issue for this is not marked as resolved since the PR above was made for another issue but the PR above resolved both.

I commented that this is resolved by the PR above; however, I opened this PR as I would like to just add
a little bit of corrections.

In the previous PR, I refactored the test by not reducing just collecting filters; however, this would not test  properly `And` filter (which is not given to the tests). I unintentionally changed this from the original way (before being refactored).

In this PR, I just followed the original way to collect filters by reducing.

I would like to close this if this PR is inappropriate and somebody would like this deal with it in the separate PR related with this.

Author: hyukjinkwon <[email protected]>

Closes #9554 from HyukjinKwon/SPARK-9557.
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