Thanks for pointing it out! Based on the discussion, I’ve created a PR: https://github.com/apache/spark/pull/49534.
Let me know what you think! On Thu, Jan 16, 2025 at 2:25 PM Xiao Li <lix...@databricks.com.invalid> wrote: > Thank you for pointing it out! Let’s update the template to exclude > documentation changes from the behavior change question. > > At the same time, I strongly believe that all behavior changes should be > clearly documented in the PR description. For instance, in the first > example PR https://github.com/apache/spark/pull/47756, the behavior > changes were not documented. It’s crucial for all committers to carefully > review the PR titles and descriptions to ensure they are accurate and > complete before merging. > > How can we bring more attention to this issue and ensure it becomes a > consistent practice? > > On Thu, Jan 16, 2025 at 1:54 PM Reynold Xin <r...@databricks.com.invalid> > wrote: > >> Seems like we should fix the template if that's not the intent .... >> >> On Thu, Jan 16, 2025 at 1:52 PM Nicholas Chammas < >> nicholas.cham...@gmail.com> wrote: >> >>> The template says "including all aspects such as the documentation fix >>> <https://github.com/apache/spark/blob/ffb31565e5af6f9ab2f8f7b500fbd595c8bb5a58/.github/PULL_REQUEST_TEMPLATE#L34-L36>.” >>> The original intent may well have been about behavior changes only, but >>> that’s not reflected in the current text of the PR template. >>> >>> >>> On Jan 16, 2025, at 2:32 PM, Dongjoon Hyun <dongjoon.h...@gmail.com> >>> wrote: >>> >>> The original intent is a user-facing *behavior* change technically >>> which is the same with Apache Spark migration guide. >>> >>> If so, does it make sense to you? >>> >>> Probably, since the template was short to be concise, it could be >>> interpreted in more ways than we thought. >>> >>> Dongjoon. >>> >>> On Thu, Jan 16, 2025 at 11:16 AM Nicholas Chammas < >>> nicholas.cham...@gmail.com> wrote: >>> >>>> I didn’t write the pull request template and I am not sure about the >>>> author’s original intent. >>>> >>>> However, the plain meaning of “any” — with the added emphasis >>>> <https://github.com/apache/spark/blob/ffb31565e5af6f9ab2f8f7b500fbd595c8bb5a58/.github/PULL_REQUEST_TEMPLATE#L34-L36> >>>> as >>>> well — suggests that the author of this policy did mean that yes, even a >>>> typo fix in a user-facing documentation page merits a “yes” response to >>>> this question. >>>> >>>> It’s strict but it’s also clear and unambiguous. No one has to think >>>> about whether their user-facing change is big enough to merit a yes. >>>> >>>> IMO this is better than a more ambiguous policy that says something >>>> like “yes if it’s user-facing _and_ is not a trivial fix”, because then >>>> there will be disagreements about what is trivial vs. not. >>>> >>>> But my opinion on the correct policy doesn’t matter that much since I’m >>>> not a committer and not a heavy reviewer. I mostly wanted to raise this so >>>> that we could somehow resolve this apparent disconnect between the plain >>>> meaning of the PR template and how PR authors are responding to it. >>>> >>>> >>>> On Jan 16, 2025, at 2:02 PM, Dongjoon Hyun <dongjoon.h...@gmail.com> >>>> wrote: >>>> >>>> I understand your concern, Nicholas. However, isn't it too strict? >>>> >>>> For the above example, adding a new HTML page is a user-facing change. >>>> >>>> https://github.com/apache/spark/pull/48852 (This is a new doc) >>>> [SPARK-50309][DOCS] Document SQL Pipe Syntax >>>> >>>> https://github.com/apache/spark/pull/49098 (This is one line link >>>> addition) >>>> [SPARK-48426][DOCS][FOLLOWUP] Add `Operators` page to `sql-ref.md` >>>> >>>> Given your definition, even a word typo fix inside an HTML page becomes >>>> a user-facing change. >>>> Did I understand your definition correctly? Or, is it something else? >>>> >>>> Dongjoon. >>>> >>>> >>>> On Thu, Jan 16, 2025 at 9:15 AM Nicholas Chammas < >>>> nicholas.cham...@gmail.com> wrote: >>>> >>>>> This is not a big deal at all, but I figure it’s worth bringing up >>>>> briefly because the pull request template does emphasize >>>>> <https://github.com/apache/spark/blob/ffb31565e5af6f9ab2f8f7b500fbd595c8bb5a58/.github/PULL_REQUEST_TEMPLATE#L34-L36> >>>>> : >>>>> >>>>> > ### Does this PR introduce _any_ user-facing change? >>>>> > >>>>> > Note that it means *any* user-facing change including all aspects >>>>> such as the documentation fix. >>>>> >>>>> And yet I regularly see PRs where the author states “no user-facing >>>>> change” even though there are clearly user-facing documentation changes >>>>> included in the diff. >>>>> >>>>> Some examples: >>>>> - https://github.com/apache/spark/pull/47756 >>>>> - https://github.com/apache/spark/pull/49098 >>>>> - https://github.com/apache/spark/pull/48852 >>>>> - https://github.com/apache/spark/pull/49273 >>>>> >>>>> Again, not a big deal. But I assume committers care about this since >>>>> it’s in the PR template. >>>>> >>>>> We should remind contributors (and each other) to answer this question >>>>> correctly when appropriate, or we should perhaps update the PR template if >>>>> the guidance there is outdated. >>>>> >>>>> Nick >>>>> >>>>> >>>> >>> > > -- > >
