Skip to content

[SQL] Simplifies binary node pattern matching #6537

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 3 commits into from
Closed

[SQL] Simplifies binary node pattern matching #6537

wants to merge 3 commits into from

Conversation

@liancheng
Copy link
Contributor

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

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

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

@liancheng liancheng force-pushed the binary-node-patmat branch from d4922e9 to d4c48fb Compare May 31, 2015 10:04
liancheng
liancheng reviewed May 31, 2015
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 is made uppercase so that it's not recognized as a free variable in pattern matching.

@liancheng
Copy link
Contributor Author

@yhuai @rxin If this looks good to you, we may merge this after #6405 and #6505 to avoid unnecessary merging conflicts for @cloud-fan.

@liancheng
Copy link
Contributor Author

Ah, just realized that @cloud-fan also adds BinaryComparison.unapply in #6405.

cloud-fan
cloud-fan reviewed May 31, 2015
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/HiveTypeCoercion.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.

Existing: why we swap the order here? StringNaN is at left but we put Literal(Double.NaN) at right. Did I missed something here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I also wondered for a while when updating this. But I think the order is irrelevant here.

Copy link
Contributor

Choose a reason for hiding this comment

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

let's do the minimum thing needed and not switch the order here. No need to confuse the future user why orders are switched.

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 order was switched in the original code, and this PR respects that. I kinda tend to switch it back to the "normal" way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea let's just switch it. BTW this PR conflicts pretty heavily with @cloud-fan's right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, they conflict, but not super heavily. That's why I suggested merging his PRs first and then this one.

@SparkQA
Copy link

SparkQA commented May 31, 2015

Test build #33849 has finished for PR 6537 at commit d4c48fb.

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

rxin
rxin reviewed May 31, 2015
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/HiveTypeCoercion.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.

unrelated to this pr, but i find 'l" somewhat hard to differentiate from "1". might be better in the future to use left vs right

Copy link
Contributor

Choose a reason for hiding this comment

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

actually it is related to this pr .. we didn't have l and r before :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, will update.

@SparkQA
Copy link

SparkQA commented Jun 1, 2015

Test build #33874 has finished for PR 6537 at commit f83d282.

  • This patch passes all tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@liancheng liancheng force-pushed the binary-node-patmat branch from f83d282 to 17c5ea5 Compare June 1, 2015 14:54
cloud-fan
cloud-fan reviewed Jun 1, 2015
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/HiveTypeCoercion.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.

left instead of l?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, these were introduced while rebasing, thanks!

@liancheng liancheng force-pushed the binary-node-patmat branch 2 times, most recently from 3e06036 to 188028f Compare June 1, 2015 15:30
@SparkQA
Copy link

SparkQA commented Jun 1, 2015

Test build #33893 has finished for PR 6537 at commit 17c5ea5.

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

@SparkQA
Copy link

SparkQA commented Jun 1, 2015

Test build #33896 has finished for PR 6537 at commit 3e06036.

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

@SparkQA
Copy link

SparkQA commented Jun 1, 2015

Test build #33898 has finished for PR 6537 at commit 188028f.

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

@liancheng liancheng force-pushed the binary-node-patmat branch from 188028f to 6b1b915 Compare June 4, 2015 08:12
@SparkQA
Copy link

SparkQA commented Jun 4, 2015

Test build #34169 has finished for PR 6537 at commit 6b1b915.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@liancheng liancheng force-pushed the binary-node-patmat branch from 6b1b915 to a3bf5fe Compare June 5, 2015 10:20
@SparkQA
Copy link

SparkQA commented Jun 5, 2015

Test build #34272 has finished for PR 6537 at commit a3bf5fe.

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

@liancheng
Copy link
Contributor Author

Since #6405 and #6505 have already been merged. I'm merging this to master.

@asfgit asfgit closed this in bc0d76a Jun 5, 2015
@liancheng liancheng deleted the binary-node-patmat branch June 5, 2015 15:08
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
@liancheng liancheng mentioned this pull request Nov 16, 2015
Closed
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 @SparkQA @rxin @cloud-fan