-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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-23303][SQL] improve the explain result for data source v2 relations #20477
Conversation
Test build #86937 has finished for PR 20477 at commit
|
Test build #86957 has finished for PR 20477 at commit
|
retest this please |
Test build #86962 has finished for PR 20477 at commit
|
Test build #86963 has finished for PR 20477 at commit
|
Test build #86974 has finished for PR 20477 at commit
|
retest this please |
Test build #86986 has finished for PR 20477 at commit
|
extends LeafExecNode with DataSourceReaderHolder with ColumnarBatchScan { | ||
|
||
override def canEqual(other: Any): Boolean = other.isInstanceOf[DataSourceV2ScanExec] | ||
|
||
override def simpleString: String = s"Scan $metadataString" |
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.
For your info, https://github.com/apache/spark/pull/20226/files#diff-3e1258979e16f72a829abb8a1cd88bda is also updating the output of the explain. Overriding the nodeName looks better for UI.
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.
+1 for overriding nodeName.
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.
I've replied on that PR. I don't think overwriting nodeName
is the right way to fix the UI issue, as we need to overwrite more methods. We can discuss more on that PR about this problem, but it should not block this PR.
Utils.truncatedString(entries.map { | ||
case (key, value) => key + ": " + StringUtils.abbreviate(redact(value), 100) | ||
}, " (", ", ", ")") | ||
} else "" |
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.
Nit. style
Test build #87064 has finished for PR 20477 at commit
|
Test build #87087 has finished for PR 20477 at commit
|
@cloud-fan
It seems to me that push down is happened at optimization. Should the optimized logical plan also contain the pushed filter like this?
|
c4bfbf4
to
c0c5895
Compare
The result was out-dated, I've updated the PR description, please check again, thanks! |
Test build #87145 has finished for PR 20477 at commit
|
Test build #87146 has finished for PR 20477 at commit
|
retest this please |
Test build #87152 has finished for PR 20477 at commit
|
retest this please |
Test build #87158 has finished for PR 20477 at commit
|
Test build #87189 has finished for PR 20477 at commit
|
also cc @tdas @jose-torres @zsxwing |
Test build #87197 has finished for PR 20477 at commit
|
retest this please |
Test build #87208 has finished for PR 20477 at commit
|
Test build #87220 has finished for PR 20477 at commit
|
Test build #87242 has finished for PR 20477 at commit
|
retest this please |
Test build #87247 has finished for PR 20477 at commit
|
retest this please |
Test build #87350 has finished for PR 20477 at commit
|
retest this please |
Test build #87358 has finished for PR 20477 at commit
|
LGTM Merged to master. |
As pointed out by @tdas , since this PR impacts the streaming, I am reverting this PR from master. Thanks! |
To be clear, the MicrobatchReader -> DataSourceV2 map added to MicroBatchExecution has potential implications in the scenario of self-joins (that I am trying to debug in #20598). |
Thanks! The PR has been reverted. |
Thank you very much @gatorsmile, I promise I will do a proper review of the streaming side when you reopen this PR. |
What changes were proposed in this pull request?
The current explain result for data source v2 relation is unreadable:
after this PR
an example for streaming query
How was this patch tested?
N/A