Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Show user a more intuitive message when queries fall back to Spark #656

Merged
merged 11 commits into from
Jul 12, 2024
Merged

feat: Show user a more intuitive message when queries fall back to Spark #656

merged 11 commits into from
Jul 12, 2024

Conversation

andygrove
Copy link
Member

Which issue does this PR close?

N/A

Rationale for this change

Previously we just listed some fallback reasons but without showing which parts of the plan caused this. Now we show a tree view. Here are before and after examples (not for the same plan).

Before

Comet cannot execute some parts of this plan natively because:
	- HashAggregate is not native because the following children are not native (Project)
	- HashAggregate is not native because the following children are not native (Exchange)
	- HashAggregate is not native because the following children are not native (HashAggregate)
	- Project is not native because the following children are not native (SortMergeJoin)

After

Comet cannot execute some parts of this plan natively:
 Exchange [COMET: Exchange is not native because the following children are not native (HashAggregate)]
+-  HashAggregate [COMET: HashAggregate is not native because the following children are not native (Project)]
   +-  Project [COMET: Project is not native because the following children are not native (SortMergeJoin)]
      +-  SortMergeJoin [COMET: SortMergeJoin is not enabled because the following children are not native (Sort)]
         :-  Sort [COMET: Sort is not native because the following children are not native (AQEShuffleRead)]
         :  +- AQEShuffleRead
         :     +-  Exchange [COMET: Exchange is not native because the following children are not native (BroadcastHashJoin)]
         :        +-  BroadcastHashJoin [COMET: BroadcastHashJoin is not enabled because the following children are not native (BroadcastExchange)]
         :           :- ColumnarToRow
         :           :  +- CometFilter
         :           :     +- CometScan parquet
         :           +- BroadcastExchange
         :              +- ColumnarToRow
         :                 +- CometProject
         :                    +- CometFilter
         :                       +- CometScan parquet
         +- CometSort
            +- AQEShuffleRead
               +- CometSinkPlaceHolder
                  +- CometExchange
                     +- CometFilter
                        +- CometScan parquet

What changes are included in this PR?

How are these changes tested?

@andygrove
Copy link
Member Author

@parthchandra fyi

kazuyukitanimura
kazuyukitanimura reviewed Jul 11, 2024
View reviewed changes
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 " +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, not sure the exact difference from COMET_EXPLAIN_VERBOSE_ENABLED... @parthchandra

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. I had tested with this set to true, but for the fallback logging we always want the verbose tree format. I will push a commit to resolve this

Copy link
Member Author

@andygrove andygrove Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the code that I introduced in this PR, which I am now going to remove, the only calls to org.apache.comet.ExtendedExplainInfo#generateExtendedInfo are from tests, so the COMET_EXPLAIN_VERBOSE_ENABLED does not actually have any impact on end users. Perhaps it should be marked as an internal config?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

parthchandra
parthchandra approved these changes Jul 12, 2024
View reviewed changes
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 " +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
"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)}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@andygrove andygrove mentioned this pull request Jul 12, 2024
Closed
13 tasks
@andygrove andygrove merged commit b6d868c into apache:main Jul 12, 2024
74 checks passed
@andygrove andygrove deleted the prettier-fallback-reason branch July 12, 2024 20:40
himadripal pushed a commit to himadripal/datafusion-comet that referenced this pull request Sep 7, 2024
@andygrove
feat: Show user a more intuitive message when queries fall back to Sp…
46e9d67 
…ark (apache#656)

* update rustfmt to reorder imports

* Enable fallback logging by default and use verbose format

* update config guide

* revert change

* revert change

* call generateVerboseExtendedInfo instead of generateExtendedInfo

* improve log message

* improve log message

* format
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@viirya viirya viirya approved these changes

@kazuyukitanimura kazuyukitanimura kazuyukitanimura approved these changes

@parthchandra parthchandra parthchandra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@andygrove @viirya @kazuyukitanimura @parthchandra