Skip to content

Commit 70caf37

Browse files
authored
Add SQL as a category in breaking API change policy (#22179)
## Which issue does this PR close? - Follow on to #21743 - Related to #22102 ## Rationale for this change As we have discovered, sometimes seemingly minor changes in SQL semantics have caused non trivial downstream pain, for example the recent changes to NULL semantics in `ANY/ALL`. See - #21743 (comment) ## What changes are included in this PR? Add a note to the the API guide making it clear that changes to SQL semantics are also API changes too and deserve extra attention. ## Are these changes tested? By CI ## Are there any user-facing changes? Clearer API docs
1 parent d91dcb7 commit 70caf37

1 file changed

Lines changed: 32 additions & 14 deletions

File tree

docs/source/contributor-guide/api-health.md

Lines changed: 32 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,13 @@ changes to avoid issues for downstream users.
2525

2626
## Breaking API Changes
2727

28-
### What is the public API and what is a breaking API change?
28+
### What is the public Rust API and what is a breaking API change?
2929

30-
In general, an item is part of the public API if it appears on the [docs.rs page].
30+
An item is part of the public Rust API if it appears on the [docs.rs page].
3131

32-
Breaking public API changes are those that _require_ users to change their code
33-
for it to compile and execute, and are listed as "Major Changes" in the [SemVer
34-
Compatibility Section of the Cargo Book]. Common examples of breaking changes include:
32+
Breaking changes _require_ users to modify their code for it to compile and
33+
run, and are listed as "Major Changes" in the [SemVer Compatibility Section of
34+
the Cargo Book]. Common examples include:
3535

3636
- Adding new required parameters to a function (`foo(a: i32, b: i32)` -> `foo(a: i32, b: i32, c: i32)`)
3737
- Removing a `pub` function
@@ -43,6 +43,16 @@ Examples of non-breaking changes include:
4343
- Marking a function as deprecated (`#[deprecated]`)
4444
- Adding a new function to a `trait` with a default implementation
4545

46+
### What is the public SQL API and what is a breaking SQL change?
47+
48+
DataFusion is also used as a SQL engine, so changes to SQL semantics (the
49+
results returned for a given query) are a form of breaking change. Even with
50+
no Rust API change, altering the behavior of an existing SQL construct can
51+
silently break downstream applications, dashboards, and tests.
52+
53+
We apply the same caution to SQL semantics changes as to Rust API changes:
54+
the benefit must be weighed against the cost of breaking downstream users.
55+
4656
### When to make breaking API changes?
4757

4858
When possible, we prefer to avoid making breaking API changes. One common way to
@@ -54,22 +64,30 @@ change with the cost (impact on downstream users). It is often frustrating for
5464
downstream users to change their applications, and it is even more so if they
5565
do not gain improved capabilities.
5666

57-
Examples of good reasons for making a breaking API change include:
67+
Examples of good reasons for a breaking API or SQL change:
5868

59-
- The change allows new use cases that were not possible before
60-
- The change significantly enables improved performance
69+
- It enables new use cases that were not possible before
70+
- It significantly improves performance
71+
- The previous behavior is clearly wrong (e.g. produces incorrect results)
6172

62-
Examples of potentially weak reasons for making breaking API changes include:
73+
Examples of potentially weak reasons:
6374

64-
- The change is an internal refactor to make DataFusion more consistent
65-
- The change is to remove an API that is not widely used but has not been marked as deprecated
75+
- An internal refactor to make DataFusion more consistent
76+
- Removing an API that is not widely used but has not been marked as deprecated
77+
- Slightly improving compatibility with another database (for example,
78+
PostgreSQL or DuckDB)
6679

6780
### What to do when making breaking API changes?
6881

69-
When making breaking public API changes, please:
82+
When making breaking Rust API changes, please:
83+
84+
1. Add the `api-change` label so the change is highlighted in the release notes.
85+
2. Document non-trivial changes in the version-specific [Upgrade Guide].
7086

71-
1. Add the `api-change` label to the PR so we can highlight the changes in the release notes.
72-
2. Consider adding documentation to the version-specific [Upgrade Guide] if the required changes are non-trivial.
87+
For breaking SQL changes, also describe the previous and new behavior in the PR
88+
description, ideally including example queries and results where appropriate.
89+
This makes review easier and helps downstream users discover the affected
90+
semantics.
7391

7492
[docs.rs page]: https://docs.rs/datafusion/latest/datafusion/index.html
7593
[semver compatibility section of the cargo book]: https://doc.rust-lang.org/cargo/reference/semver.html#change-categories

0 commit comments

Comments
 (0)