[SPARK-28710][SQL]to fix replace function, spark should call drop and create function #25452

sandeep-katta · 2019-08-14T09:51:18Z

What changes were proposed in this pull request?

create or replace function X as 'Y' using jar Z; does not work if the X is already present and is created with some other jar lets say xyz.

As per current implementation spark calls alter Function API of Hive, as of now Hive only alter name, owner, class name, type but not resource URI. After calling alter function only X and Y are updated but not Z.

So when the select is performed on X UDF it throws class not found exception.

Observation 1: Temporary function does not have this problem as it is handled by spark logic

Observation 2: For permanent function Spark calls the Hive to alter function , as of now Hive only alter name, owner, class name, type but not resource URI.

[SparkCode]

[Hive Code]

As per Hive Documentation it does not supports create or replace command for function

How was this patch tested?

Added UT and also manually tested

Before Fix:

After Fix:

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLQuerySuite.scala

sql/core/src/main/scala/org/apache/spark/sql/execution/command/functions.scala

dongjoon-hyun

I left a few comments, @sandeep-katta . Thank you for your contribution.

maropu · 2019-08-14T23:44:20Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLQuerySuite.scala

+        """.stripMargin)
+      val cnt =
+        sql("SELECT customAdd(2, 'A', 10, date '2015-01-01', 'B', 20, date '2016-01-01')").count()
+      assert(cnt === 2)


nit: How about checking actual answers instread of the row count?

maropu · 2019-08-15T00:01:03Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLQuerySuite.scala

      }
    }
  }
+  test("SPARK-28710: Replace permanent function ") {


This test should be in HiveUDFSuite?

viirya · 2019-08-15T00:20:58Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/functions.scala

      // Handles `CREATE OR REPLACE FUNCTION AS ... USING ...`
      if (replace && catalog.functionExists(func.identifier)) {
-        // alter the function in the metastore
-        catalog.alterFunction(func)


Instead of changing CreateFunctionCommand, should we change the behavior at HiveExternalCatalog.alterFunction? If this is for Hive only?

HiveExternalCatalog is for Hive only, currently alter function of Hive does not update the resource

@viirya do I need to handle any other cases for this PR ?

Yeah, I think @viirya's point is valid. Isn't the root cause that HiveExternalCatalog.alterFunction does not handle?

If this is for Hive only?

I think he meant the behaviours at InMemoryCatalog.alterFunction.

I got it, yes this problem is only for HiveExternalCatalog, I will update the code and push

sandeep-katta · 2019-08-18T02:40:43Z

I left a few comments, @sandeep-katta . Thank you for your contribution.

I have fixed the comments

HyukjinKwon · 2019-09-17T00:35:36Z

ok to test

SparkQA · 2019-09-17T00:58:39Z

Test build #110697 has finished for PR 25452 at commit f9ba185.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

sandeep-katta · 2019-09-17T18:39:44Z

retest this please

HyukjinKwon · 2019-09-18T03:55:04Z

retest this please

SparkQA · 2019-09-18T07:05:02Z

Test build #110861 has finished for PR 25452 at commit f9ba185.

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

maropu · 2019-09-18T07:14:38Z

retest this please

SparkQA · 2019-09-18T09:34:10Z

Test build #110879 has finished for PR 25452 at commit f9ba185.

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

Remove jar and move testcase to HiveUDFSuite. Also added check for change in the resource

SparkQA · 2019-12-16T15:33:41Z

Test build #115391 has finished for PR 25452 at commit ebf7868.

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

viirya · 2019-12-16T16:22:45Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala

    client.listFunctions(db, pattern)
  }

+  def isResourceChanged(db: String, newFunction: CatalogFunction): Boolean = {


viirya · 2019-12-16T16:24:13Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala

+      case true => client.dropFunction(db, functionName)
+        client.createFunction(db, newDefinition)


this style looks uncommon in Spark. We have it usually:

isResourceChanged(db, newDefinition) match { case true => client.dropFunction(db, functionName) client.createFunction(db, newDefinition) ... }

viirya · 2019-12-16T16:28:22Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala

+    val changedResources = newFunction.resources.filter(
+      newResource => !oldFunction.resources.contains(newResource))


Will case sensitivity be problem in this comparison? Shall we lower case before comparison?

I think it should be case sensitive, cz in case of unix based system /opt/some.jar is different from /OPT/SOME.jar. Isn't it ?

viirya · 2019-12-16T16:34:47Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala

+        client.createFunction(db, newDefinition)
+      // replace the function in the metastore if there is no change in the resource
+      case _ =>
+        client.alterFunction(db, newDefinition)


From Hive DDL, looks like it does not have "replace" in create function:

CREATE FUNCTION [db_name.]function_name AS class_name [USING JAR|FILE|ARCHIVE 'file_uri' [, JAR|FILE|ARCHIVE 'file_uri'] ];

Maybe it is why this API does not do resource replacement?

yes, as of now Hive only alter name, owner, class name, type but not resource URI. There is no replace DDL in Hive

And also initially in the jira, I proposed 2 solutions for this bug

solution 1: throw Unsupported Error for permanent function
solution 2: instead of alter function, do drop and create

SparkQA · 2019-12-16T17:07:29Z

Test build #115390 has finished for PR 25452 at commit 05f3b7b.

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

SparkQA · 2019-12-17T07:02:58Z

Test build #115427 has finished for PR 25452 at commit 95d1e23.

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

sandeep-katta · 2020-01-07T04:37:00Z

ping @viirya

github-actions · 2020-06-21T00:24:05Z

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

dongjoon-hyun added the SQL label Aug 14, 2019

dongjoon-hyun reviewed Aug 14, 2019

View reviewed changes

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLQuerySuite.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Aug 14, 2019

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/command/functions.scala Outdated Show resolved Hide resolved

dongjoon-hyun requested changes Aug 14, 2019

View reviewed changes

maropu reviewed Aug 14, 2019

View reviewed changes

maropu reviewed Aug 15, 2019

View reviewed changes

viirya reviewed Aug 15, 2019

View reviewed changes

to fix replace function, spark should call drop and create function

2843cb3

Remove jar and move testcase to HiveUDFSuite. Also added check for change in the resource

sandeep-katta force-pushed the alterfunction branch from f9ba185 to 05f3b7b Compare December 16, 2019 13:01

moved definition to HiveExteralCatalog

ebf7868

sandeep-katta force-pushed the alterfunction branch from 05f3b7b to ebf7868 Compare December 16, 2019 13:12

viirya reviewed Dec 16, 2019

View reviewed changes

fixed review comments

95d1e23

github-actions bot added the Stale label Jun 21, 2020

github-actions bot closed this Jun 22, 2020

		case true => client.dropFunction(db, functionName)
		client.createFunction(db, newDefinition)

		val changedResources = newFunction.resources.filter(
		newResource => !oldFunction.resources.contains(newResource))

[SPARK-28710][SQL]to fix replace function, spark should call drop and create function #25452

[SPARK-28710][SQL]to fix replace function, spark should call drop and create function #25452

Uh oh!

Conversation

sandeep-katta commented Aug 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Uh oh!

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandeep-katta commented Aug 18, 2019

Uh oh!

HyukjinKwon commented Sep 17, 2019

Uh oh!

SparkQA commented Sep 17, 2019

Uh oh!

sandeep-katta commented Sep 17, 2019

Uh oh!

HyukjinKwon commented Sep 18, 2019

Uh oh!

SparkQA commented Sep 18, 2019

Uh oh!

maropu commented Sep 18, 2019

Uh oh!

SparkQA commented Sep 18, 2019

Uh oh!

SparkQA commented Dec 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 16, 2019

Uh oh!

SparkQA commented Dec 17, 2019

Uh oh!

sandeep-katta commented Jan 7, 2020

Uh oh!

github-actions bot commented Jun 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sandeep-katta commented Aug 14, 2019 •

edited

Loading