doc: Add initial doc how to expand Comet exceptions #170

comphead · 2024-03-06T01:24:16Z

Which issue does this PR close?

Closes #104 .

Rationale for this change

Adding docs how to extend Comet exceptions by leveraging DF backtraces

What changes are included in this PR?

How are these changes tested?

comphead · 2024-03-06T01:25:51Z

@snmvaughan cc

sunchao · 2024-03-06T05:15:08Z

DEBUGGING.md

+Start Comet with `RUST_BACKTRACE=1`
+
+```commandline
+RUST_BACKTRACE=1 $SPARK_HOME/spark-shell --jars spark/target/comet-spark-spark3.4_2.12-0.1.0-SNAPSHOT.jar --conf spark.sql.extensions=org.apache.comet.CometSparkSessionExtensions --conf spark.comet.enabled=true --conf spark.comet.exec.enabled=true --conf spark.comet.exec.all.enabled=true


Just curious, if we don't enable RUST_BACKTRACE=1, is there any performance penalty from enabling backtrace?

Same here. If there's no penalty without enabling RUST_BACKTRACE=1, I think we can build with backtrace enabled and disabled it at runtime.

Just curious, if we don't enable RUST_BACKTRACE=1, is there any performance penalty from enabling backtrace?

We had some discussion back in the day on this apache/datafusion#7522 (comment)

The problem was with analyzer, and in Comet we use physical plan, however it might still have some overhead IMHO, as it has to call Backtrace::enabled function which will def take some cpu cycles

advancedxy

wow, thanks for this doc. I think it's super useful.

advancedxy · 2024-03-06T12:15:46Z

DEBUGGING.md

+```
+Note: 
+- The backtrace coverage in Datafusion is still improving. So there is a chance the error still not covered, feel free to file a [ticket](https://github.com/apache/arrow-datafusion/issues)
+- The backtrace doesn't come for free and therefore intended for debugging purposes


Curious to know, what percentage of overhead are we talking about? Like 1%, 5% or something 10%?

advancedxy · 2024-03-06T12:17:10Z

DEBUGGING.md

+To do that with Comet it is needed to enable `backtrace` in  https://github.com/apache/arrow-datafusion-comet/blob/main/core/Cargo.toml
+
+```
+datafusion-common = { version = "36.0.0", features = ["backtrace"] }


Seems like we should update the cargo.toml to include backtrace feature by default for local/debug build?

advancedxy · 2024-03-06T12:18:36Z

DEBUGGING.md

+Start Comet with `RUST_BACKTRACE=1`
+
+```commandline
+RUST_BACKTRACE=1 $SPARK_HOME/spark-shell --jars spark/target/comet-spark-spark3.4_2.12-0.1.0-SNAPSHOT.jar --conf spark.sql.extensions=org.apache.comet.CometSparkSessionExtensions --conf spark.comet.enabled=true --conf spark.comet.exec.enabled=true --conf spark.comet.exec.all.enabled=true


Same here. If there's no penalty without enabling RUST_BACKTRACE=1, I think we can build with backtrace enabled and disabled it at runtime.

advancedxy · 2024-03-06T12:20:58Z

DEBUGGING.md

+Start Comet with `RUST_BACKTRACE=1`
+
+```commandline
+RUST_BACKTRACE=1 $SPARK_HOME/spark-shell --jars spark/target/comet-spark-spark3.4_2.12-0.1.0-SNAPSHOT.jar --conf spark.sql.extensions=org.apache.comet.CometSparkSessionExtensions --conf spark.comet.enabled=true --conf spark.comet.exec.enabled=true --conf spark.comet.exec.all.enabled=true


A little off the topic. For distributed environments, I believe --conf spark.executorEnv.RUST_BACKTRACE=1 should be passed to make sure executor's JVM is started with backtrace enabled.

That is good idea if we decide to keep backtrace enabled always

comphead · 2024-03-06T16:43:52Z

If we ok with the initial text lets merge it and I'll create a followup to measure performance with backtrace feature enabled/disabled

advancedxy · 2024-03-06T17:18:23Z

DEBUGGING.md

@@ -30,6 +30,62 @@ LLDB or the LLDB that is bundled with XCode. We will use the LLDB packaged with

 _Caveat: The steps here have only been tested with JDK 11_ on Mac (M1)

+# Expand Comet exception details


It seems a bit weird to put this between the first paragraph and the section of Debugging for Advanced Developers which are both talking about IDEs..

How about put this part after Additional Info?

Otherwise, thos doc LGTM.

sunchao

LGTM

viirya · 2024-03-06T18:09:31Z

DEBUGGING.md

+
+```
+
+There is a verbose exception option by leveraging Datafusion [backtraces](https://arrow.apache.org/datafusion/user-guide/example-usage.html#enable-backtraces)


Suggested change

There is a verbose exception option by leveraging Datafusion [backtraces](https://arrow.apache.org/datafusion/user-guide/example-usage.html#enable-backtraces)

There is a verbose exception option by leveraging DataFusion [backtraces](https://arrow.apache.org/datafusion/user-guide/example-usage.html#enable-backtraces)

viirya · 2024-03-06T18:09:58Z

DEBUGGING.md

+```
+
+There is a verbose exception option by leveraging Datafusion [backtraces](https://arrow.apache.org/datafusion/user-guide/example-usage.html#enable-backtraces)
+This option allows to append native Datafusion stacktrace to the original error message. 


Suggested change

This option allows to append native Datafusion stacktrace to the original error message.

This option allows to append native DataFusion stacktrace to the original error message.

viirya · 2024-03-06T18:10:17Z

DEBUGGING.md

+
+There is a verbose exception option by leveraging Datafusion [backtraces](https://arrow.apache.org/datafusion/user-guide/example-usage.html#enable-backtraces)
+This option allows to append native Datafusion stacktrace to the original error message. 
+To enable this option with Comet it is needed to include `backtrace` feature in [Cargo.toml](https://github.com/apache/arrow-datafusion-comet/blob/main/core/Cargo.toml) for Datafusion dependencies


Suggested change

To enable this option with Comet it is needed to include `backtrace` feature in [Cargo.toml](https://github.com/apache/arrow-datafusion-comet/blob/main/core/Cargo.toml) for Datafusion dependencies

To enable this option with Comet it is needed to include `backtrace` feature in [Cargo.toml](https://github.com/apache/arrow-datafusion-comet/blob/main/core/Cargo.toml) for DataFusion dependencies

viirya · 2024-03-06T18:11:06Z

DEBUGGING.md

+...
+```
+Note:
+- The backtrace coverage in Datafusion is still improving. So there is a chance the error still not covered, feel free to file a [ticket](https://github.com/apache/arrow-datafusion/issues)


Suggested change

- The backtrace coverage in Datafusion is still improving. So there is a chance the error still not covered, feel free to file a [ticket](https://github.com/apache/arrow-datafusion/issues)

- The backtrace coverage in DataFusion is still improving. So there is a chance the error still not covered, feel free to file a [ticket](https://github.com/apache/arrow-datafusion/issues).

viirya · 2024-03-06T18:11:35Z

DEBUGGING.md

+```
+Note:
+- The backtrace coverage in Datafusion is still improving. So there is a chance the error still not covered, feel free to file a [ticket](https://github.com/apache/arrow-datafusion/issues)
+- The backtrace doesn't come for free and therefore intended for debugging purposes


Suggested change

- The backtrace doesn't come for free and therefore intended for debugging purposes

- The backtrace doesn't come for free and therefore intended for debugging purposes.

I'm afraid that "doesn't come for free" reads a bit ambiguous. Maybe change to more explicit way like "The backtrace comes with performance cost ...".

comphead · 2024-03-06T18:23:15Z

Addressed all the comments, thanks folks for helping with the review. I plan to merge it within 1 hr, if no other comments

doc: Add initial doc how to expand Comet exceptions

49d84f8

comphead requested review from viirya and sunchao March 6, 2024 01:24

sunchao reviewed Mar 6, 2024

View reviewed changes

advancedxy reviewed Mar 6, 2024

View reviewed changes

doc: Add initial doc how to expand Comet exceptions. refmt

04d7ac7

comphead requested review from sunchao and advancedxy March 6, 2024 18:01

sunchao approved these changes Mar 6, 2024

View reviewed changes

advancedxy approved these changes Mar 6, 2024

View reviewed changes

viirya reviewed Mar 6, 2024

View reviewed changes

viirya approved these changes Mar 6, 2024

View reviewed changes

doc: Add initial doc how to expand Comet exceptions. refmt

c2f2d9b

comphead merged commit 0915342 into apache:main Mar 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: Add initial doc how to expand Comet exceptions #170

doc: Add initial doc how to expand Comet exceptions #170

comphead commented Mar 6, 2024

comphead commented Mar 6, 2024

sunchao Mar 6, 2024

advancedxy Mar 6, 2024

comphead Mar 6, 2024

advancedxy left a comment

advancedxy Mar 6, 2024

advancedxy Mar 6, 2024

advancedxy Mar 6, 2024

advancedxy Mar 6, 2024

comphead Mar 6, 2024

comphead commented Mar 6, 2024

advancedxy Mar 6, 2024

sunchao left a comment

viirya Mar 6, 2024

viirya Mar 6, 2024

viirya Mar 6, 2024

viirya Mar 6, 2024 •

edited

Loading

viirya Mar 6, 2024

viirya Mar 6, 2024

comphead commented Mar 6, 2024

		@@ -30,6 +30,62 @@ LLDB or the LLDB that is bundled with XCode. We will use the LLDB packaged with

		_Caveat: The steps here have only been tested with JDK 11_ on Mac (M1)

		# Expand Comet exception details


		```

		There is a verbose exception option by leveraging Datafusion [backtraces](https://arrow.apache.org/datafusion/user-guide/example-usage.html#enable-backtraces)

	This option allows to append native Datafusion stacktrace to the original error message.
	This option allows to append native DataFusion stacktrace to the original error message.

	To enable this option with Comet it is needed to include `backtrace` feature in [Cargo.toml](https://github.com/apache/arrow-datafusion-comet/blob/main/core/Cargo.toml) for Datafusion dependencies
	To enable this option with Comet it is needed to include `backtrace` feature in [Cargo.toml](https://github.com/apache/arrow-datafusion-comet/blob/main/core/Cargo.toml) for DataFusion dependencies

	- The backtrace coverage in Datafusion is still improving. So there is a chance the error still not covered, feel free to file a [ticket](https://github.com/apache/arrow-datafusion/issues)
	- The backtrace coverage in DataFusion is still improving. So there is a chance the error still not covered, feel free to file a [ticket](https://github.com/apache/arrow-datafusion/issues).

	- The backtrace doesn't come for free and therefore intended for debugging purposes
	- The backtrace doesn't come for free and therefore intended for debugging purposes.

doc: Add initial doc how to expand Comet exceptions #170

doc: Add initial doc how to expand Comet exceptions #170

Conversation

comphead commented Mar 6, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

comphead commented Mar 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

advancedxy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comphead commented Mar 6, 2024

Choose a reason for hiding this comment

sunchao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comphead commented Mar 6, 2024

viirya Mar 6, 2024 •

edited

Loading