andygrove commented on code in PR #7250: URL: https://github.com/apache/datafusion-comet/pull/7250#discussion_r4543464764
########## docs/source/contributor-guide/upgrading_datafusion.md: ########## @@ -0,0 +1,137 @@ +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +# Upgrading DataFusion + +A DataFusion major release usually changes APIs Comet calls, often moves to new major versions of +the crates whose types cross DataFusion's API (today Arrow, Parquet and object_store), and sometimes +changes behavior that Comet's tests depend on. So while the next DataFusion major version is in +development, Comet builds against DataFusion `main` on a long-lived upgrade branch, which surfaces +breaks and regressions while there is still time to fix them upstream. + +## The Upgrade Branch + +The upgrade to DataFusion N lives on a branch named `datafusion-N` (for example, `datafusion-56`) +in `apache/datafusion-comet` itself, not on a fork, so any committer can push to it. One draft pull +request from that branch to `main` carries the upgrade, titled `deps: upgrade to DataFusion N.0.0`. +It stays a draft until DataFusion N.0.0 is released (see [Merging the Upgrade](#merging-the-upgrade)). + +An EPIC issue titled `DataFusion N upgrade` tracks the upgrade, with each regression it finds as a +sub-issue. The pull request links the EPIC. + +Keep the branch as a small number of commits on top of current `main`, so the pull request shows +only the upgrade. Updating it means resetting onto `main` and force pushing, as described in +[Submitting a Pull Request](development.md#submitting-a-pull-request). + +## Pinning DataFusion Main + +The DataFusion crates point at one DataFusion `main` revision with `git` and `rev`, not a version: + +```toml +datafusion = { git = "https://github.com/apache/datafusion";, rev = "<sha>", default-features = false, features = [...] } +``` + +Use the same revision in every manifest that names a DataFusion crate, including the dev-dependencies +and the contrib crates, which have their own manifests outside the workspace: + +```shell +git grep -l 'datafusion' -- '*Cargo.toml' +``` + +Move every crate whose types cross DataFusion's API to the version that revision uses, then +regenerate every lockfile (`git ls-files '*Cargo.lock'`). Only one major version of each may be in +the build, because Comet passes their types directly between DataFusion and other crates. Check it +in `native/` and in each contrib directory with its own lockfile: + +```shell +cargo tree -i arrow --workspace --all-features --depth 0 +``` + +This fails with "multiple `arrow` packages" if two versions are in the build. + +A dependency that is still on the old version, such as iceberg-rust, gets pinned to a revision that +has the upgrade, such as an open upstream pull request or a fork branch built from one. Leave a +comment in `Cargo.toml` that names the upstream pull request and says when to move back. + +When moving to a newer DataFusion revision, make the change and fix the build locally, push it +once, and record the new revision and its date in the pull request description. + +## Fixing the Build + +Read DataFusion's upgrade guide for the release, +`docs/source/library-user-guide/upgrading/N.0.0.md` in the DataFusion repository at the pinned +revision, before fixing compile errors. It lists the API changes and the intended replacements. + +Keep source changes to the ones the upgrade forces. A cleanup that the new version enables, such as +removing a workaround for a bug that is now fixed upstream, goes in a follow-up pull request after +the upgrade merges, and the upgrade pull request lists it. + +When an API Comet calls is deprecated without a replacement, keep the call under +`#[allow(deprecated)]` with a comment, and file a DataFusion issue describing Comet's use case +before the API is removed. + +The upgrade guide also lists behavior changes, such as different function return types or +different handling of statistics. Check each one against Comet, and record in the pull request +description whether and why it affects Comet. Many don't, because Comet registers its own versions +of many functions. + +## Running the Tests + +Locally, work through the checklist in +[Submitting a Pull Request](development.md#submitting-a-pull-request), and build `native/` with +every `contrib-*` feature from `native/core/Cargo.toml` enabled, since the contrib crates are +outside the default build. Leave the Comet suites to CI. + +The pull request's default checks don't cover the other Spark versions, Spark's own SQL tests, or +the Iceberg and Delta suites, and a new DataFusion version can change results in any of them. Once +the build is clean on a new DataFusion revision, add the labels from +[Continuous Integration](ci.md#opting-a-pull-request-into-a-suite-the-pr-tier-skips) that run them: +`run-all-spark-profiles`, `run-iceberg-tests`, `run-delta-tests`, and the Spark SQL tests label for +the default Spark version. A label stays on the pull request and reruns its suite on every push, so +remove the labels once their runs report. + +## Handling Regressions + +A test that fails on the new DataFusion revision and passes on `main` is a regression. For each one: + +1. Find the cause and the project that owns it. It may be DataFusion, or a change in a dependency + that moved with it. Some upstream changes are deliberate, so check the upstream pull request + before calling it a bug. +2. File a Comet issue as a sub-issue of the EPIC, describing the difference from Spark and the + upstream cause. +3. Fix it in the project that owns it. A fix that merges to DataFusion `main` before the release + branch is cut ships in N.0.0. If the fix belongs in Comet, make it on the upgrade branch. +4. If the fix won't land before the upgrade merges, ignore the test with a reason that links the Review Comment: Agreed, an ignored test is not enough for a wrong result that stays enabled. Step 4 now has two cases. If the path is already safe (Comet falls back, or the path is off by default or behind an incompatible opt-in), the test is ignored with the issue link and the limitation is documented. If a default-enabled native expression or operator now returns a different result from Spark, the upgrade has to make it fall back or move it behind an incompatible opt-in first, and if neither is possible it is deferred until the fix lands. The merge steps ask to check that no ignored test hides such a result (4739a25c8d). ########## docs/source/contributor-guide/upgrading_datafusion.md: ########## @@ -0,0 +1,137 @@ +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +# Upgrading DataFusion + +A DataFusion major release usually changes APIs Comet calls, often moves to new major versions of +the crates whose types cross DataFusion's API (today Arrow, Parquet and object_store), and sometimes +changes behavior that Comet's tests depend on. So while the next DataFusion major version is in +development, Comet builds against DataFusion `main` on a long-lived upgrade branch, which surfaces +breaks and regressions while there is still time to fix them upstream. + +## The Upgrade Branch + +The upgrade to DataFusion N lives on a branch named `datafusion-N` (for example, `datafusion-56`) +in `apache/datafusion-comet` itself, not on a fork, so any committer can push to it. One draft pull +request from that branch to `main` carries the upgrade, titled `deps: upgrade to DataFusion N.0.0`. +It stays a draft until DataFusion N.0.0 is released (see [Merging the Upgrade](#merging-the-upgrade)). + +An EPIC issue titled `DataFusion N upgrade` tracks the upgrade, with each regression it finds as a +sub-issue. The pull request links the EPIC. + +Keep the branch as a small number of commits on top of current `main`, so the pull request shows +only the upgrade. Updating it means resetting onto `main` and force pushing, as described in +[Submitting a Pull Request](development.md#submitting-a-pull-request). Review Comment: That is right, the lease compares with the ref that was just fetched, so it would not catch it. I took your second option. The branch is updated by merging `main` into it, the pull request still shows only the upgrade, and the squash merge collapses the history. The section says not to reset and force push, why, and that if a reset is needed it is announced on the EPIC first, with a request that pending commits are rebased onto the new head (4739a25c8d). -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
