Skip to content

[SPARK-24544][SQL][FOLLOWUP] Remove a wrong warning on Hive fallback lookup #24773

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
dongjoon-hyun wants to merge 1 commit into from
Closed

[SPARK-24544][SQL][FOLLOWUP] Remove a wrong warning on Hive fallback lookup #24773

dongjoon-hyun wants to merge 1 commit into from

Conversation

@dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Jun 3, 2019 •
edited
Loading

What changes were proposed in this pull request?

This PR is a follow-up of #21790 which causes a regression to show misleading warnings always at first invocation for all Hive function. Hive fallback lookup should not be warned. It's a normal process in function lookups.

CURRENT (Showing NoSuchFunctionException and working)

scala> sql("select histogram_numeric(a,2) from values(1) T(a)").show
19/06/02 22:02:10 WARN HiveSessionCatalog: Encountered a failure during looking up
function: org.apache.spark.sql.catalyst.analysis.NoSuchFunctionException: 
Undefined function: 'histogram_numeric'. This function is neither a registered temporary
function nor a permanent function registered in the database 'default'.;
  at org.apache.spark.sql.catalyst.catalog.SessionCatalog.failFunctionLookup(SessionCatalog.scala:1234)
  at org.apache.spark.sql.catalyst.catalog.SessionCatalog.lookupFunction(SessionCatalog.scala:1302)
...
+------------------------+
|histogram_numeric( a, 2)|
+------------------------+
|            [[1.0, 1.0]]|
+------------------------+

How was this patch tested?

Manually execute the above query.

dongjoon-hyun
dongjoon-hyun commented Jun 3, 2019
View reviewed changes
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveSessionCatalog.scala
// Hive is case insensitive.
val functionName = funcName.unquotedString.toLowerCase(Locale.ROOT)
logWarning("Encountered a failure during looking up function:" +
s" ${Utils.exceptionString(error)}")
Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jun 3, 2019

Choose a reason for hiding this comment

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

We should not warn here.

Copy link
Member

@felixcheung felixcheung Jun 3, 2019

Choose a reason for hiding this comment

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

maybe this is intended for the true case of if (super.functionExists(name)) above? like L138?

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jun 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Ya. Maybe, but I'm not sure about the original intention. For that line 138, since we already throw error, actually, we don't need print warning here with the same error.

felixcheung
felixcheung approved these changes Jun 3, 2019
View reviewed changes
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveSessionCatalog.scala
// Hive is case insensitive.
val functionName = funcName.unquotedString.toLowerCase(Locale.ROOT)
logWarning("Encountered a failure during looking up function:" +
s" ${Utils.exceptionString(error)}")
Copy link
Member

@felixcheung felixcheung Jun 3, 2019

Choose a reason for hiding this comment

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

maybe this is intended for the true case of if (super.functionExists(name)) above? like L138?

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jun 3, 2019

Thank you for review and approval, @felixcheung .

gatorsmile
gatorsmile reviewed Jun 3, 2019
View reviewed changes
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

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jun 3, 2019

Thank you for review and approval, @gatorsmile !

HyukjinKwon
HyukjinKwon approved these changes Jun 3, 2019
View reviewed changes
Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

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

One trivial question. LGTM as is

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveSessionCatalog.scala
// let's try to load it as a Hive's built-in function.
// Hive is case insensitive.
val functionName = funcName.unquotedString.toLowerCase(Locale.ROOT)
logWarning("Encountered a failure during looking up function:" +
Copy link
Member

@HyukjinKwon HyukjinKwon Jun 3, 2019

Choose a reason for hiding this comment

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

Why don't we debug or info that we'll try fallback while we're here?

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jun 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. Of course, we can add debug messages any places. But, this is a natural flow for Hive built-in function lookup. To showing a message like Encountered a failure during looking up function: NoSuchFunctionException for every Hive function at the first invocation will not give much values to the developer/users. Also, it's misleading to the users because Spark will execute the function without any problem at the same exeuction.

Copy link
Member

@HyukjinKwon HyukjinKwon Jun 3, 2019

Choose a reason for hiding this comment

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

I meant to add a debug message like "Hive fallback will be attempted for the function [abc]." because users/dev might not know which function exist in Spark and which function comes from Hive or at least where the fallback happens.

This warn should be removed anyway ..

Copy link
Member

@HyukjinKwon HyukjinKwon Jun 3, 2019

Choose a reason for hiding this comment

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

let's talk about this somewhere else later. I think it's not a big deal at all. LGTM it should be removed anyway.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jun 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I see, @HyukjinKwon .
For that message, I guess that the reason why the code doesn't have debug/info originally is that the Apache Spark's ambition was to implement everything in Spark-side and to remove this fallback logic completely. :)

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jun 3, 2019

Thank you for review and approval, @HyukjinKwon .

@SparkQA
Copy link

SparkQA commented Jun 3, 2019

Test build #106091 has finished for PR 24773 at commit 5eac93d.

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

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jun 3, 2019

Merged to master. Thank you all!

@dongjoon-hyun dongjoon-hyun deleted the SPARK-24544 branch June 3, 2019 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HyukjinKwon HyukjinKwon HyukjinKwon approved these changes

@gatorsmile gatorsmile gatorsmile left review comments

@felixcheung felixcheung felixcheung approved these changes

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dongjoon-hyun @SparkQA @HyukjinKwon @felixcheung @gatorsmile