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.

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

[SPARK-22384][SQL][followup] Refine partition pruning when attribute is wrapped in Cast #21712

Closed
wants to merge 2 commits into from

Conversation

cloud-fan
Copy link
Contributor

What changes were proposed in this pull request?

As mentioned in #21586 , Cast.mayTruncate is not 100% safe, string to boolean is allowed. Since changing Cast.mayTruncate also changes the behavior of Dataset, here I propose to add a new Cast.canSafeCast for partition pruning.

How was this patch tested?

new test cases

@cloud-fan
Copy link
Contributor Author

cc @jinxing64 @gatorsmile @viirya

"aa" :: "ab" :: "ba" :: "bb" :: Nil)
}

test("getPartitionsByFilter: cast(chunk as boolean)=1 (not a valid partition predicate)") {
Copy link
Member

@viirya viirya Jul 4, 2018

Choose a reason for hiding this comment

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

nit: cast(chunk as boolean)=true

* Returns true iff we can safely cast the `from` type to `to` type without any truncating or
* precision lose, e.g. int -> long, date -> timestamp.
*/
def canSafeCast(from: AtomicType, to: AtomicType): Boolean = (from, to) match {
Copy link
Member

Choose a reason for hiding this comment

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

Is this only applied for AtomicType?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, at least for now. Hive partition predicate doesn't support complex type.

@@ -122,6 +122,22 @@ class HiveClientSuite(version: String)
"aa" :: Nil)
}

test("getPartitionsByFilter: cast(chunk as int)=1 (not a valid partition predicate)") {
Copy link
Member

Choose a reason for hiding this comment

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

I ran this test with current master branch, seems it can also pass without this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes this can pass, we can tell it by #12586

Cast.mayTruncate returns true for string to int, but not string to boolean. Here I just wanna test both of the cases, to improve test coverage.

@SparkQA
Copy link

SparkQA commented Jul 4, 2018

Test build #92602 has finished for PR 21712 at commit 7b316ff.

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

@SparkQA
Copy link

SparkQA commented Jul 4, 2018

Test build #92608 has finished for PR 21712 at commit f7699ac.

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

Copy link
Member

@gatorsmile gatorsmile left a comment

Choose a reason for hiding this comment

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

LGTM

@gatorsmile
Copy link
Member

Thanks! Merged to master.

@asfgit asfgit closed this in bf764a3 Jul 5, 2018
@jinxing64
Copy link

Thanks for ping me. LGTM

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.

5 participants