You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "peter-toth (via GitHub)" <gi...@apache.org> on 2025/07/16 10:55:07 UTC
[PR] [SPARK-52820][SQL] Capture full plans in golden files [spark]
peter-toth opened a new pull request, #51512:
URL: https://github.com/apache/spark/pull/51512
### What changes were proposed in this pull request?
Current plan golden files contain truncated plans like https://github.com/apache/spark/pull/51462/files#diff-0c65bc882e994dfec3fa04b3374c0e3c69f2d8300220465d34892a33fb8911a4R1, which might not show the actual change.
### Why are the changes needed?
Plan golden files should capture the whole plan.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Existing UTs.
### 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-52820][SQL] Capture full plans in golden files [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #51512:
URL: https://github.com/apache/spark/pull/51512#discussion_r2210488871
##########
sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestHelper.scala:
##########
@@ -117,7 +119,9 @@ trait SQLQueryTestHelper extends Logging {
if (deterministic) {
// Perform query analysis, but also get rid of the #1234 expression IDs that show up in the
// resolved plans.
- (schema, Seq(replaceNotIncludedMsg(analyzed.toString)))
+ withSQLConf(SQLConf.MAX_TO_STRING_FIELDS.key -> "100") {
Review Comment:
100 is a magic number for now, @peter-toth ? I'm curious if there is an issue when we use the maximum value there.
--
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-52820][SQL] Capture full plans in golden files [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #51512: [SPARK-52820][SQL] Capture full plans in golden files
URL: https://github.com/apache/spark/pull/51512
--
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-52820][SQL] Capture full plans in golden files [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #51512:
URL: https://github.com/apache/spark/pull/51512#discussion_r2210510129
##########
sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestHelper.scala:
##########
@@ -117,7 +119,9 @@ trait SQLQueryTestHelper extends Logging {
if (deterministic) {
// Perform query analysis, but also get rid of the #1234 expression IDs that show up in the
// resolved plans.
- (schema, Seq(replaceNotIncludedMsg(analyzed.toString)))
+ withSQLConf(SQLConf.MAX_TO_STRING_FIELDS.key -> "100") {
Review Comment:
maximum is ok ~
--
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-52820][SQL] Capture full plans in golden files [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #51512:
URL: https://github.com/apache/spark/pull/51512#discussion_r2210491446
##########
sql/connect/server/src/test/scala/org/apache/spark/sql/connect/ProtoToParsedPlanTestSuite.scala:
##########
@@ -198,7 +199,9 @@ class ProtoToParsedPlanTestSuite
}
Helper.execute(catalystPlan)
}
- val actual = removeMemoryAddress(normalizeExprIds(finalAnalyzedPlan).treeString)
+ val actual = withSQLConf(SQLConf.MAX_TO_STRING_FIELDS.key -> "100") {
Review Comment:
ditto. Why 100?
--
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-52820][SQL] Capture full plans in golden files [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #51512:
URL: https://github.com/apache/spark/pull/51512#discussion_r2210505744
##########
sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestHelper.scala:
##########
@@ -117,7 +119,9 @@ trait SQLQueryTestHelper extends Logging {
if (deterministic) {
// Perform query analysis, but also get rid of the #1234 expression IDs that show up in the
// resolved plans.
- (schema, Seq(replaceNotIncludedMsg(analyzed.toString)))
+ withSQLConf(SQLConf.MAX_TO_STRING_FIELDS.key -> "100") {
Review Comment:
Yeah, we can increase it to the maximum: https://github.com/apache/spark/pull/51512/commits/7c9f3ee04270ba83e7c1339134aa42bde2211324
--
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-52820][SQL] Capture full plans in golden files [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #51512:
URL: https://github.com/apache/spark/pull/51512#issuecomment-3082667086
Thank you all for the reivew.
--
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-52820][SQL] Capture full plans in golden files [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #51512:
URL: https://github.com/apache/spark/pull/51512#discussion_r2210488871
##########
sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestHelper.scala:
##########
@@ -117,7 +119,9 @@ trait SQLQueryTestHelper extends Logging {
if (deterministic) {
// Perform query analysis, but also get rid of the #1234 expression IDs that show up in the
// resolved plans.
- (schema, Seq(replaceNotIncludedMsg(analyzed.toString)))
+ withSQLConf(SQLConf.MAX_TO_STRING_FIELDS.key -> "100") {
Review Comment:
100 is a magic number for now, @peter-toth ?
--
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-52820][SQL] Capture full plans in golden files [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #51512:
URL: https://github.com/apache/spark/pull/51512#issuecomment-3078428847
cc @dongjoon-hyun
--
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