parthchandra commented on code in PR #656:
URL: https://github.com/apache/datafusion-comet/pull/656#discussion_r1674936633
##########
common/src/main/scala/org/apache/comet/CometConf.scala:
##########
@@ -286,9 +286,10 @@ object CometConf extends ShimCometConf {
conf("spark.comet.explainFallback.enabled")
.doc(
"When this setting is enabled, Comet will provide logging explaining
the reason(s) " +
- "why a query stage cannot be executed natively.")
+ "why a query stage cannot be executed natively. Set this to false to
" +
Review Comment:
> I just remembered that this `ExtendedExplainInfo` is implementing a Spark
API that the Spark UI can call in Spark 4. Setting
COMET_EXPLAIN_VERBOSE_ENABLED will show the tree view there.
That is correct. We should not change the default (yet). By default the user
will see the abbreviated version in the Spark UI. Setting
`COMET_EXPLAIN_VERBOSE_ENABLED` to true will show the verbose version in the
Spark UI.
For our internal logging purposes we can explicitly call
`ExtendedExplainInfo.generateVerboseExtendedInfo`
##########
spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala:
##########
@@ -850,17 +850,13 @@ class CometSparkSessionExtensions
// if the plan cannot be run fully natively then explain why (when
appropriate
// config is enabled)
if (CometConf.COMET_EXPLAIN_FALLBACK_ENABLED.get()) {
- new ExtendedExplainInfo().extensionInfo(newPlan) match {
- case reasons if reasons.size == 1 =>
- logWarning(
- "Comet cannot execute some parts of this plan natively " +
- s"because ${reasons.head}")
- case reasons if reasons.size > 1 =>
- logWarning(
- "Comet cannot execute some parts of this plan natively" +
- s" because:\n\t- ${reasons.mkString("\n\t- ")}")
- case _ =>
- // no reasons recorded
+ val info = new ExtendedExplainInfo()
+ if (info.extensionInfo(newPlan).nonEmpty) {
+ logWarning(
+ "Comet cannot execute some parts of this plan natively " +
+ s"(set ${CometConf.COMET_EXPLAIN_FALLBACK_ENABLED.key}=false "
+
+ "to disable this logging):\n" +
+ s"${info.generateVerboseExtendedInfo(newPlan)}")
Review Comment:
This can generate a very large amount of text if the plan is very large. But
I suppose the user can disable the output if they want to.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
