-
Notifications
You must be signed in to change notification settings - Fork 198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add a behavior change guideline #518
Conversation
https://spark.apache.org/versioning-policy.html There is a section 'Considerations when breaking APIs' on the version policy page, shall we merge them together to avoid divergence and redundancy? |
breaking API is really a special case of behavior change, and should be fine to be documented separately. With Spark Connect, breaking API is more about the client while behavior change is more about the server. |
contributing.md
Outdated
- Any non-additive change to the public Python/SQL/Scala/Java/R APIs (including developer APIs): rename function, | ||
remove parameters, add parameters, rename parameters, change parameter default values, etc. These changes should | ||
be avoided in general, or done in a binary-compatible way like deprecating and adding a new function instead of renaming. | ||
- Any non-additive change to the way Spark should be deployed and managed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not an example. Maybe an actual example?
contributing.md
Outdated
know about these correctness fixes. | ||
- Bug fixes that change query schema. Users may need to update the schema of the tables in their data pipelines | ||
and must know about these changes. | ||
- Remove configs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spark config?
contributing.md
Outdated
explicitly in the PR description. If the behavior change may require additional user actions, we should mention | ||
it in the migration guide and if possible add a legacy config to restore the behavior. Some examples: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the migration guide, and how should the user add this information? Can you include that information?
Co-authored-by: Niranjan <[email protected]>
contributing.md
Outdated
- Bug fixes that change query schema. Users may need to update the schema of the tables in their data pipelines | ||
and must know about these changes. | ||
- Remove configs. | ||
- Rename error class/condition. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does changing error message itself count (not touch the error class name and condition, but rewording the error message)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, error message change is OK (think about we support different languages in the future)
contributing.md
Outdated
|
||
Behavior changes are user-visible functional changes in a new release via public APIs. The "user" here is not | ||
only the user who writes queries and/or develops Spark plugins, but also the user who deploys and/or manages | ||
Spark clusters. New features and bug fixes that for example eliminate `NullPointerException` or correct query |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eliminate
NullPointerException
Let us use a different example
contributing.md
Outdated
know about these correctness fixes. | ||
- Bug fixes that change query schema. Users may need to update the schema of the tables in their data pipelines | ||
and must know about these changes. | ||
- Remove Spark configurations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove / rename
contributing.md
Outdated
version upgrades. If a PR introduces behavior changes, it should be called out explicitly in the PR description. | ||
If the behavior change may require additional user actions, this should be called out in the migration guide | ||
(`docs/sql-migration-guide.md` for SQL component and similar files for other components) and where possible | ||
provide options to restore the previous behavior. Some examples: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, the error messages needs to explain how to restore to the previous behavior .
As we discussed in the dev list (in the thread "[DISCUSS] clarify the definition of behavior changes"), we need a clear definition of behavior changes so that contributors can follow.
screenshot: