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.

Sign up for GitHub

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

Jump to bottom

[SPARK-13092][SQL] Add ExpressionSet for constraint tracking #11338

Closed
wants to merge 2 commits into from
Closed

[SPARK-13092][SQL] Add ExpressionSet for constraint tracking #11338

wants to merge 2 commits into from

Conversation

marmbrus
Copy link
Contributor

This PR adds a new abstraction called an ExpressionSet which attempts to canonicalize expressions to remove cosmetic differences. Deterministic expressions that are in the set after canonicalization will always return the same answer given the same input (i.e. false positives should not be possible). However, it is possible that two canonical expressions that are not equal will in fact return the same answer given any input (i.e. false negatives are possible).

val set = AttributeSet('a + 1 :: 1 + 'a :: Nil)

set.iterator => Iterator('a + 1)
set.contains('a + 1) => true
set.contains(1 + 'a) => true
set.contains('a + 2) => false

Other relevant changes include:

  • Since this concept overlaps with the existing semanticEquals and semanticHash, those functions are also ported to this new infrastructure.
  • A memoized canonicalized version of the expression is added as a lazy val to Expression and is used by both semanticEquals and ExpressionSet.
  • A set of unit tests for ExpressionSet are added
  • Tests which expect semanticEquals to be less intelligent than it now is are updated.

As a followup, we should consider auditing the places where we do O(n) semanticEquals operations and replace them with ExpressionSet. We should also consider consolidating AttributeSet as a specialized factory for an ExpressionSet.

@marmbrus
Copy link
Contributor Author

/cc @sameeragarwal @yhuai

@marmbrus
Copy link
Contributor Author

test this please

@SparkQA
Copy link

SparkQA commented Feb 24, 2016

Test build #51829 has finished for PR 11338 at commit 2a4f4de.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class ExpressionSet protected(

yhuai
yhuai reviewed Feb 24, 2016
View reviewed changes
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionSetSuite.scala
assert((initialSet + (aUpper + 1)).size == 1)
assert((initialSet + (aUpper + 2)).size == 2)
assert((initialSet - (aUpper + 1)).size == 0)
assert((initialSet - (aUpper + 2)).size == 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Also test (initialSet + (aLower + 1)).size and (initialSet - (aLower + 1)).size?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

marmbrus
marmbrus reviewed Feb 24, 2016
View reviewed changes
sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
verifyCallCount(
df.selectExpr("testUdf(a + 1) + testUdf(1 + a)", "testUdf(a + 1)"), Row(4, 2), 2)
df.selectExpr("testUdf(a + 1) + testUdf(1 + a)", "testUdf(a + 1)"), Row(4, 2), 1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

/cc @nongli

@marmbrus
Copy link
Contributor Author

test this please

2 similar comments
@marmbrus
Copy link
Contributor Author

test this please

@marmbrus
Copy link
Contributor Author

test this please

@yhuai
Copy link
Contributor

yhuai commented Feb 24, 2016

LGTM

@yhuai
Copy link
Contributor

yhuai commented Feb 24, 2016

test this please

@SparkQA
Copy link

SparkQA commented Feb 25, 2016

Test build #51909 has finished for PR 11338 at commit a255d95.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Feb 25, 2016

Test build #2581 has finished for PR 11338 at commit a255d95.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@yhuai
Copy link
Contributor

yhuai commented Feb 25, 2016

test this please

@SparkQA
Copy link

SparkQA commented Feb 25, 2016

Test build #51917 has finished for PR 11338 at commit a255d95.

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

@yhuai
Copy link
Contributor

yhuai commented Feb 25, 2016

Thanks. Merging to master.

@asfgit asfgit closed this in 2b04257 Feb 25, 2016
@marmbrus marmbrus deleted the expressionSet branch March 8, 2016 00:04
@sameeragarwal sameeragarwal mentioned this pull request Mar 9, 2016
Closed
asfgit pushed a commit that referenced this pull request Mar 9, 2016
@sameeragarwal @marmbrus
[SPARK-13781][SQL] Use ExpressionSets in ConstraintPropagationSuite
dbf2a7c 
## What changes were proposed in this pull request?

This PR is a small follow up on #11338 (https://issues.apache.org/jira/browse/SPARK-13092) to use `ExpressionSet` as part of the verification logic in `ConstraintPropagationSuite`.
## How was this patch tested?

No new tests added. Just changes the verification logic in `ConstraintPropagationSuite`.

Author: Sameer Agarwal <sameer@databricks.com>

Closes #11611 from sameeragarwal/expression-set.
roygao94 pushed a commit to roygao94/spark that referenced this pull request Mar 22, 2016
@sameeragarwal @roygao94
[SPARK-13781][SQL] Use ExpressionSets in ConstraintPropagationSuite
363d879 
## What changes were proposed in this pull request?

This PR is a small follow up on apache#11338 (https://issues.apache.org/jira/browse/SPARK-13092) to use `ExpressionSet` as part of the verification logic in `ConstraintPropagationSuite`.
## How was this patch tested?

No new tests added. Just changes the verification logic in `ConstraintPropagationSuite`.

Author: Sameer Agarwal <sameer@databricks.com>

Closes apache#11611 from sameeragarwal/expression-set.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@marmbrus @SparkQA @yhuai