[SQL][MINOR][Refactor] Refactor on sql/core#22685
[SQL][MINOR][Refactor] Refactor on sql/core#22685da-liii wants to merge 2 commits intoapache:masterfrom
Conversation
|
Test build #97189 has finished for PR 22685 at commit
|
da-liii
changed the title
HyukjinKwon
left a comment
HyukjinKwon
left a comment
There was a problem hiding this comment.
Looks mostly style changes. I wouldn't do this since it makes backporting harder. I think it's best to help review and leave some comments for such nits in the future.
|
@HyukjinKwon OK, most of changes are related to parens. I will consider to reset it back for backporting friendliness. |
da-liii
changed the title
|
I think you're mostly trying to target changes styles. If there is not actual benefit rather then just styles, I wouldn't do this and just help review other PRs. |
| buildStaticConf("spark.sql.ui.retainedExecutions") | ||
| .doc("Number of executions to retain in the Spark UI.") | ||
| .intConf | ||
| .longConf |
There was a problem hiding this comment.
This involves a implicit int2long cast.
| val builder = new StringBuilder | ||
| val kFields = kExprEnc.schema.map { | ||
| case f => s"${f.name}: ${f.dataType.simpleString(2)}" | ||
| f => s"${f.name}: ${f.dataType.simpleString(2)}" |
There was a problem hiding this comment.
Should not be a partial function!
| case 's' => | ||
| // Read StructType for DataFrame | ||
| val fields = SerDe.readList(dis, jvmObjectTracker = null).asInstanceOf[Array[Object]] | ||
| val fields = SerDe.readList(dis, jvmObjectTracker = null) |
There was a problem hiding this comment.
useless asInstanceOf
| def createMetric(sc: SparkContext, name: String): SQLMetric = { | ||
| val acc = new SQLMetric(SUM_METRIC) | ||
| acc.register(sc, name = Some(name), countFailedValues = false) | ||
| acc.register(sc, name = Some(name)) |
There was a problem hiding this comment.
duplicated assignment
| dataType: DataType, | ||
| inputSchemas: Option[Seq[ScalaReflection.Schema]]): UserDefinedFunction = { | ||
| val udf = new UserDefinedFunction(f, dataType, inputSchemas.map(_.map(_.dataType))) | ||
| val udf = UserDefinedFunction(f, dataType, inputSchemas.map(_.map(_.dataType))) |
There was a problem hiding this comment.
useless new for case class
|
@HyukjinKwon Sorry. Happened to find that some code is not elegant according to my taste. I I have commented on the most important ones according to my standard. Please review. And the rest, I will revert. |
|
Test build #97194 has finished for PR 22685 at commit
|
HyukjinKwon
left a comment
HyukjinKwon
left a comment
There was a problem hiding this comment.
It still looks mostly just style changes. Touching multiple files here and there brings backporting overhead in the future (say, this diff will causes some conflicts when we backport some patches into other branches that does not have this change). If the change is not worth enough like just style, I wouldn't go ahead.
There's one legitimate change that might cause a potential actual problem (I commented above), which I wouldn't propose a PR to fix one instance
| private val equality = sparkSession.sessionState.conf.resolver | ||
|
|
||
| bucketSpec.map { bucket => | ||
| bucketSpec.foreach { bucket => |
There was a problem hiding this comment.
Yea, this is legitimate change.
|
OK. Nevermind. |
3 participants
What changes were proposed in this pull request?
Only minor changes on Scala syntax.
How was this patch tested?
Existing Tests