tanelk commented on a change in pull request #31927:
URL: https://github.com/apache/spark/pull/31927#discussion_r598935673
##########
File path:
sql/core/src/test/resources/tpcds-plan-stability/approved-plans-v2_7/q70a.sf100/explain.txt
##########
@@ -207,7 +207,7 @@ Arguments: [rank(_w2#17) windowspecdefinition(s_state#10,
_w2#17 DESC NULLS LAST
(32) Filter [codegen id : 7]
Input [4]: [s_state#16, s_state#10, _w2#17, ranking#19]
-Condition : (isnotnull(ranking#19) AND (ranking#19 <= 5))
Review comment:
To me it seems beneficial to see, that how your PR affects real query
plans. From the diff of this PR it looks like there are about 4 changes to the
`explain.txt`, that did not show up in the `simplified.txt`:
- Some filter conditions are deduplicated -
`GreaterThan(hd_vehicle_count,0)` from q34.
- There are some odd whitespace changes - perhaps it would have been useful
to catch them during the PR, that caused them.
- The date representation has changed 0 -> 1970-01-01
- the `isnotnull` change from my PR.
Perhaps I missed some, but it does not seem like the `explain.txt` changes
too often.
But my main argument for this change is that it is very confusing if you
make a change, that should show up in the `explain.txt` after running
`SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "sql/testOnly
*PlanStability[WithStats]Suite"`, but it does not. As was the case, when I
changed the `Rank` to be not nullable.
And now if someone else would make a change, that impacts the
`simplified.txt`, then they would get some unexpected/unrelated changes also to
their `explain.txt`.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
