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-48041][SQL] Avoid call conf.resolver repeated in TableOutputResolver #46279
base: master
Are you sure you want to change the base?
Conversation
@dongjoon-hyun PTAL |
@@ -3816,7 +3816,7 @@ class Analyzer(override val catalogManager: CatalogManager) extends RuleExecutor | |||
case u: UnresolvedFieldPosition => u.position match { | |||
case after: After => | |||
val allFields = parentSchema.fieldNames ++ fieldsAdded | |||
allFields.find(n => conf.resolver(n, after.column())) match { | |||
allFields.find(n => resolver(n, after.column())) match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's one more below L3895
Can you fill the PR description please? |
Done @HyukjinKwon |
Is there any other outstanding issue similar to SPARK-48010 and this one? |
yes,similar Sences@yaooqinn |
expected.forall(e => queryOutput.exists(p => conf.resolver(p.name, e.name))) && | ||
expected.zip(queryOutput).exists(e => !conf.resolver(e._1.name, e._2.name))) { | ||
expected.forall(e => queryOutput.exists(p => resolver(p.name, e.name))) && | ||
expected.zip(queryOutput).exists(e => !resolver(e._1.name, e._2.name))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this okay? What happens spark.sql.caseSensitive
is changed during session?
@@ -219,7 +221,7 @@ object TableOutputResolver extends SQLConfHelper with Logging { | |||
fillDefaultValue: Boolean = false): Seq[NamedExpression] = { | |||
val matchedCols = mutable.HashSet.empty[String] | |||
val reordered = expectedCols.flatMap { expectedCol => | |||
val matched = inputCols.filter(col => conf.resolver(col.name, expectedCol.name)) | |||
val matched = inputCols.filter(col => resolver(col.name, expectedCol.name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This conf
is given by the method parameter. I'm not sure this is correct optimization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To @xuzifu666 , TableOutputResolver
Scala object is created once for JVM, isn't it?
I'm wondering if this proposal is correct or not. Basically, the user can change conf.resolver
at any time, but this proposal reads it only once.
WDYT?
What changes were proposed in this pull request?
This PR aim to call conf.resolver for each call in Analyzer.
Why are the changes needed?
conf.resolver be called repeated in TableOutputResolver,with change can reuse the same resolver.
Does this PR introduce any user-facing change?
none
How was this patch tested?
no need
Was this patch authored or co-authored using generative AI tooling?
no