Skip to content

Conversation

@huaxingao
Copy link
Contributor

@huaxingao huaxingao commented Sep 17, 2017

What changes were proposed in this pull request?

org.apache.spark.sql.jdbc.JdbcDialect's method:
def isCascadingTruncateTable(): Option[Boolean] = None
is not overriden in org.apache.spark.sql.jdbc.AggregatedDialect class.
Because of this issue, when you add more than one dialect Spark doesn't truncate table because isCascadingTruncateTable always returns default None for Aggregated Dialect.
Will implement isCascadingTruncateTable in AggregatedDialect class in this PR.

How was this patch tested?

In JDBCSuite, inside test("Aggregated dialects"), will add one line to test AggregatedDialect.isCascadingTruncateTable

@gatorsmile
Copy link
Member

ok to test

}

override def isCascadingTruncateTable(): Option[Boolean] = {
dialects.flatMap(_.isCascadingTruncateTable).headOption
Copy link
Member

Choose a reason for hiding this comment

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

Why using the first one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both getCatalystType and getJDBCType(dt: DataType) use the first one. Also, in the class header, it has the following:
/**

  • AggregatedDialect can unify multiple dialects into one virtual Dialect.
  • Dialects are tried in order, and the first dialect that does not return a
  • neutral element will will.
  • @param dialects List of dialects.
    */
    It has a typo, I guess it means "the first dialect that does not return a
    neutral element will return"

Copy link
Member

Choose a reason for hiding this comment

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

This one is different from getCatalystType and getJDBCType . Instead, we should follow canHandle.

@SparkQA
Copy link

SparkQA commented Sep 17, 2017

Test build #81848 has finished for PR 19256 at commit 7b9d6fc.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@gatorsmile
Copy link
Member

retest this please

@gatorsmile
Copy link
Member

BTW, could you update the PR title?

@SparkQA
Copy link

SparkQA commented Sep 17, 2017

Test build #81859 has finished for PR 19256 at commit 7b9d6fc.

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

@huaxingao
Copy link
Contributor Author

Thanks @gatorsmile
I will change both the implementation and the PR title.

@gatorsmile
Copy link
Member

Let me correct what I said above. The logics should be

If any dialect's isCascadingTruncateTable returns true, we should return true.

@huaxingao
Copy link
Contributor Author

Thanks @gatorsmile
Does the following logic look good to you?

if(any dialect's isCascadingTruncateTable returns true)
  return Some(true)
else
  if (any dialect's isCascadingTruncateTable returns false)
     return Some(false)
  else // None of the dialect implements this method, return the superclass default value
     return None

@gatorsmile
Copy link
Member

It looks good, but the actual code should be very simple if you are writing using the Scala way

@huaxingao huaxingao changed the title [SPARK-21338][SQL]implement isCascadingTruncateTable() method in Aggr… [SPARK-21338][SQL]implement isCascadingTruncateTable() method in AggregatedDialect Sep 18, 2017
@gatorsmile
Copy link
Member

LGTM pending Jenkins

@SparkQA
Copy link

SparkQA commented Sep 19, 2017

Test build #81898 has finished for PR 19256 at commit bd8e2b9.

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

@asfgit asfgit closed this in d5aefa8 Sep 19, 2017
@gatorsmile
Copy link
Member

Thanks! Merged to master

@huaxingao
Copy link
Contributor Author

@gatorsmile Thanks a lot for your help!!!

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