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

Closed
dongjoon-hyun wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveSessionCatalog.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,6 @@ private[sql] class HiveSessionCatalog(
// 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. :)

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.

if (!hiveFunctions.contains(functionName)) {
failFunctionLookup(funcName, Some(error))
}
Expand Down