You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "alex35736 (via GitHub)" <gi...@apache.org> on 2024/03/21 13:40:15 UTC
[PR] [SPARK-47503][SQL] escaping grap node name if there is no metrics to … [spark]
alex35736 opened a new pull request, #45640:
URL: https://github.com/apache/spark/pull/45640
What changes were proposed in this pull request?
To prevent corruption of dot file a node name should be escaped even if there is no metrics to display
Why are the changes needed?
This pr fixes a bug in spark history server which fails to display query for cached JDBC relation named in quotes.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Manually test.
Was this patch authored or co-authored using generative AI tooling?
No.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47503][SQL] Make `makeDotNode` escape graph node name always [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45640:
URL: https://github.com/apache/spark/pull/45640#issuecomment-2016718644
All tests passed. Merged to master/3.5/3.4 for Apache Spark 4.0.0/3.5.2/3.4.3.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47503][SQL] escaping graph node name if there is no metrics to … [spark]
Posted by "alex35736 (via GitHub)" <gi...@apache.org>.
alex35736 commented on PR #45640:
URL: https://github.com/apache/spark/pull/45640#issuecomment-2016446333
@dongjoon-hyun Yes, sure. I should've begin with unit test Just didn't really think such a small change was worth one. Thank you for your time
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47503][SQL] Make `makeDotNode` escape graph node name always [spark]
Posted by "alex35736 (via GitHub)" <gi...@apache.org>.
alex35736 commented on PR #45640:
URL: https://github.com/apache/spark/pull/45640#issuecomment-2016835824
@dongjoon-hyun thank you for your help, i did build project locally but with test disabled, for some reason i unable to run majority of core sql tests in my local environment maybe because i'm on windows (still figuring it out) so i run this unit test separately and locally scalastyle did not raise any error so i thought it was fine.
Here is a backport https://github.com/apache/spark/pull/45684
Again thank you for help!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47503][SQL] Make `makeDotNode` escape graph node name always [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45640:
URL: https://github.com/apache/spark/pull/45640#issuecomment-2016719850
FYI, you had better provide a valid email in your PR commit. The current one seems to be invalid to me.
```
commit 15ea9de88b8d8a87daa6abbc8e80c439e2d38e03 (HEAD -> master, apache/master, apache/HEAD)
Author: Alexey <alexey13>
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47503][SQL] Make `makeDotNode` escape graph node name always [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45640:
URL: https://github.com/apache/spark/pull/45640#issuecomment-2016718902
There is a conflict in old branches.
Could you make backport PRs for `branch-3.5` and `branch-3.4`, @alex35736 ?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47503][SQL] Make `makeDotNode` escape graph node name always [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #45640:
URL: https://github.com/apache/spark/pull/45640#discussion_r1536717209
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/ui/SparkPlanGraphSuite.scala:
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.execution.ui
+
+import org.apache.spark.SparkFunSuite
+
+class SparkPlanGraphSuite extends SparkFunSuite {
+
+ test("SPARK-47503: name of a node should be escaped even if there is no metrics") {
+
Review Comment:
Please remove this too.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/ui/SparkPlanGraphSuite.scala:
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.execution.ui
+
+import org.apache.spark.SparkFunSuite
+
+class SparkPlanGraphSuite extends SparkFunSuite {
+
+ test("SPARK-47503: name of a node should be escaped even if there is no metrics") {
+
+ val planGraphNode = new SparkPlanGraphNode(
+ id = 24,
+ name = "Scan JDBCRelation(\"test-schema\".tickets) [numPartitions=1]",
+ desc = "Scan JDBCRelation(\"test-schema\".tickets) [numPartitions=1] " +
+ "[ticket_no#0] PushedFilters: [], ReadSchema: struct<ticket_no:string>",
+ metrics = List(
+ SQLPlanMetric(
+ name = "number of output rows",
+ accumulatorId = 75,
+ metricType = "sum",
+ ),
+ SQLPlanMetric(
+ name = "JDBC query execution time",
+ accumulatorId = 35,
+ metricType = "nsTiming",
+ ),
+ )
+ )
+
+ val dotNode = planGraphNode.makeDotNode(Map.empty[Long, String])
+
+ val expectedDotNode = " 24 [id=\"node24\" labelType=\"html\" label=\"" +
+ "<br><b>Scan JDBCRelation(\\\"test-schema\\\".tickets) [numPartitions=1]</b><br><br>\" " +
+ "tooltip=\"Scan JDBCRelation(\\\"test-schema\\\".tickets) [numPartitions=1] [ticket_no#0] " +
+ "PushedFilters: [], ReadSchema: struct<ticket_no:string>\"];"
+
+ assertResult(expectedDotNode)(dotNode)
+ }
+
Review Comment:
ditto. Please remove this.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47503][SQL] Make `makeDotNode` escape graph node name always [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #45640: [SPARK-47503][SQL] Make `makeDotNode` escape graph node name always
URL: https://github.com/apache/spark/pull/45640
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47503][SQL] Make `makeDotNode` escape graph node name always [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #45640:
URL: https://github.com/apache/spark/pull/45640#discussion_r1536717198
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/ui/SparkPlanGraphSuite.scala:
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.execution.ui
+
+import org.apache.spark.SparkFunSuite
+
+class SparkPlanGraphSuite extends SparkFunSuite {
Review Comment:
Could you run `dev/lint-scala` and fix the CI failures?
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/ui/SparkPlanGraphSuite.scala:
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.execution.ui
+
+import org.apache.spark.SparkFunSuite
+
+class SparkPlanGraphSuite extends SparkFunSuite {
+
Review Comment:
nit. Please remove this redundant empty line.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47503][SQL] Make `makeDotNode` escape graph node name always [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45640:
URL: https://github.com/apache/spark/pull/45640#issuecomment-2016719535
BTW,
Welcome to the Apache Spark community!
I added you (JIRA ID: alex_seko) to the Apache Spark JIRA contributor group and assigned SPARK-47503 to you.
Congratulations for your first commit.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47503][SQL] Make `makeDotNode` escape graph node name always [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45640:
URL: https://github.com/apache/spark/pull/45640#issuecomment-2016652770
Thank you for adding a test coverage, @alex35736 . If CI passed, we can merge this.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47503][SQL] Make `makeDotNode` escape graph node name always [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45640:
URL: https://github.com/apache/spark/pull/45640#issuecomment-2016688557
Let me fix like the following because those are obvious mistakes.
```scala
SQLPlanMetric(
name = "number of output rows",
accumulatorId = 75,
- metricType = "sum",
+ metricType = "sum"
),
SQLPlanMetric(
name = "JDBC query execution time",
accumulatorId = 35,
- metricType = "nsTiming",
- ),
+ metricType = "nsTiming"
+ )
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org