Skip to content

[SQL] Refactors data type pattern matching #2764

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
Closed
wants to merge 2 commits into from
Closed

[SQL] Refactors data type pattern matching #2764

wants to merge 2 commits into from

Conversation

@liancheng
Copy link
Contributor

Refactors/adds extractors for DataType and Binary* types to ease and simplify data type related (nested) pattern matching.

Review on Reviewable

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@SparkQA
Copy link

SparkQA commented Oct 11, 2014

QA tests have started for PR 2764 at commit f391be5.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Oct 11, 2014

QA tests have finished for PR 2764 at commit f391be5.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class InSet(value: Expression, hset: HashSet[Any], child: Seq[Expression])

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21640/Test PASSed.

liancheng
liancheng reviewed Oct 11, 2014
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/HiveTypeCoercion.scala Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case branch can never be reached since line 114 supersedes it. As a result, "NaN" is always Double, bug?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is probably a copy paste bug.

@marmbrus
Copy link
Contributor

This does look like a better style that what we were doing before. We should coordinate with the changes @mateiz is making for decimal type.

marmbrus
marmbrus reviewed Oct 20, 2014
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/types/dataTypes.scala Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these type parameterized? Things seems to compile without this added complexity. Extra refinement can be done regardless with nested pattern matching.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, this type parameter can be removed. Thanks.

@liancheng
Copy link
Contributor Author

Rebased to master.

@liancheng
Copy link
Contributor Author

@marmbrus This should be ready to go since #2983 has been merged.

@marmbrus
Copy link
Contributor

Sorry for the delay here. I think it would be good to clean up how we do pattern matching and this looks like a good start. Here's what I propose:

  • close this issue for now
  • write up a short description in the PR on the flavors of matching that we are going to support for datatype and expressions.
  • reopen this and we'll merge it quickly this time :)

@asfgit asfgit closed this in ca12608 Dec 17, 2014
@liancheng liancheng deleted the datatype-patmat branch December 18, 2014 04:30
@liancheng liancheng mentioned this pull request May 31, 2015
Closed
asfgit pushed a commit that referenced this pull request Jun 5, 2015
@liancheng
[SQL] Simplifies binary node pattern matching
bc0d76a 
This PR is a simpler version of #2764, and adds `unapply` methods to the following binary nodes for simpler pattern matching:

- `BinaryExpression`
- `BinaryComparison`
- `BinaryArithmetics`

This enables nested pattern matching for binary nodes. For example, the following pattern matching

```scala
case p: BinaryComparison if p.left.dataType == StringType &&
                            p.right.dataType == DateType =>
  p.makeCopy(Array(p.left, Cast(p.right, StringType)))
```

can be simplified to

```scala
case p  BinaryComparison(l  StringType(), r  DateType()) =>
  p.makeCopy(Array(l, Cast(r, StringType)))
```

Author: Cheng Lian <[email protected]>

Closes #6537 from liancheng/binary-node-patmat and squashes the following commits:

a3bf5fe [Cheng Lian] Fixes compilation error introduced while rebasing
b738986 [Cheng Lian] Renames `l`/`r` to `left`/`right` or `lhs`/`rhs`
14900ae [Cheng Lian] Simplifies binary node pattern matching
jeanlyn pushed a commit to jeanlyn/spark that referenced this pull request Jun 12, 2015
@liancheng @jeanlyn
[SQL] Simplifies binary node pattern matching
bf09340 
This PR is a simpler version of apache#2764, and adds `unapply` methods to the following binary nodes for simpler pattern matching:

- `BinaryExpression`
- `BinaryComparison`
- `BinaryArithmetics`

This enables nested pattern matching for binary nodes. For example, the following pattern matching

```scala
case p: BinaryComparison if p.left.dataType == StringType &&
                            p.right.dataType == DateType =>
  p.makeCopy(Array(p.left, Cast(p.right, StringType)))
```

can be simplified to

```scala
case p  BinaryComparison(l  StringType(), r  DateType()) =>
  p.makeCopy(Array(l, Cast(r, StringType)))
```

Author: Cheng Lian <[email protected]>

Closes apache#6537 from liancheng/binary-node-patmat and squashes the following commits:

a3bf5fe [Cheng Lian] Fixes compilation error introduced while rebasing
b738986 [Cheng Lian] Renames `l`/`r` to `left`/`right` or `lhs`/`rhs`
14900ae [Cheng Lian] Simplifies binary node pattern matching
nemccarthy pushed a commit to nemccarthy/spark that referenced this pull request Jun 19, 2015
@liancheng @nemccarthy
[SQL] Simplifies binary node pattern matching
8c8d34a 
This PR is a simpler version of apache#2764, and adds `unapply` methods to the following binary nodes for simpler pattern matching:

- `BinaryExpression`
- `BinaryComparison`
- `BinaryArithmetics`

This enables nested pattern matching for binary nodes. For example, the following pattern matching

```scala
case p: BinaryComparison if p.left.dataType == StringType &&
                            p.right.dataType == DateType =>
  p.makeCopy(Array(p.left, Cast(p.right, StringType)))
```

can be simplified to

```scala
case p  BinaryComparison(l  StringType(), r  DateType()) =>
  p.makeCopy(Array(l, Cast(r, StringType)))
```

Author: Cheng Lian <[email protected]>

Closes apache#6537 from liancheng/binary-node-patmat and squashes the following commits:

a3bf5fe [Cheng Lian] Fixes compilation error introduced while rebasing
b738986 [Cheng Lian] Renames `l`/`r` to `left`/`right` or `lhs`/`rhs`
14900ae [Cheng Lian] Simplifies binary node pattern matching
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

Development

Successfully merging this pull request may close these issues.

4 participants

@liancheng @AmplabJenkins @SparkQA @marmbrus