[SPARK-35552][SQL] Make query stage materialized more readable #32689
Conversation
|
Kubernetes integration test starting |
|
@ulysses-you, the docker test failure should be now fixed in the latest master branch. |
|
Kubernetes integration test status success |
|
thank you @HyukjinKwon , I will retriggered the GA. |
| val dataSize = runtimeStats.sizeInBytes.max(0) | ||
| val numOutputRows = runtimeStats.rowCount.map(_.max(0)) | ||
| Statistics(dataSize, numOutputRows, isRuntime = true) | ||
| def computeStats(): Option[Statistics] = { |
There was a problem hiding this comment.
nit:
def computeStats(): Option[Statistics] = if (isMaterialized) {
...
} else {
...
}
| } | ||
|
|
||
| @transient | ||
| @volatile | ||
| protected var _resultOption = new AtomicReference[Option[Any]](None) | ||
|
|
||
| private[adaptive] def resultOption: AtomicReference[Option[Any]] = _resultOption | ||
| private[adaptive] def isMaterialized: Boolean = resultOption.get().isDefined |
There was a problem hiding this comment.
materialize is a public method, I think it makes more sense to make isMaterialized public as well.
There was a problem hiding this comment.
I'm ok with it, but just wonder which code place may use isMaterialized without adaptive ?
|
Kubernetes integration test starting |
|
Kubernetes integration test unable to build dist. exiting with code: 1 |
|
Test build #139043 has finished for PR 32689 at commit
|
|
Kubernetes integration test status success |
|
Test build #139052 has finished for PR 32689 at commit
|
|
Thanks, merging to master |
|
thanks for merging ! |
What changes were proposed in this pull request?
Add a new method
isMaterializedinQueryStageExec.Why are the changes needed?
Currently, we use
resultOption().get.isDefinedto check if a query stage has materialized. The code is not readable at a glance. It's better to use a new method likeisMaterializedto define it.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Pass CI.