refactor!: remove serde support for proto types (#159)

- Removes `serde` support for the generated proto types - Uses `pbjson` ser/de when the `serde` feature is enabled. - Removes the `pbjson` feature. Closes #158.
substrait-io · Mar 7, 2024 · 72e7589 · 72e7589
1 parent d8b73ee
commit 72e7589
Show file tree

Hide file tree

Showing 7 changed files with 120 additions and 170 deletions.
diff --git a/.github/workflows/docs.yml b/.github/workflows/docs.yml
@@ -22,10 +22,21 @@ jobs:
           fetch-depth: 0
           submodules: recursive
       - uses: dtolnay/rust-toolchain@nightly
-      - uses: arduino/setup-protoc@v3
+        id: rust-toolchain
+      - uses: actions/cache@v4
         with:
-          repo-token: ${{ secrets.GITHUB_TOKEN }}
-      - run: cargo rustdoc --features semver -- --cfg docsrs
+          path: |
+            ~/.cargo/bin/
+            ~/.cargo/registry/index/
+            ~/.cargo/registry/cache/
+            ~/.cargo/git/db/
+            target/
+          key: ${{ runner.os }}-cargo-${{ steps.rust-toolchain.outputs.cachekey }}-rustdoc-${{ hashFiles('**/Cargo.toml') }}
+          restore-keys: |
+            ${{ runner.os }}-cargo-${{ steps.rust-toolchain.outputs.cachekey }}-rustdoc-
+            ${{ runner.os }}-cargo-${{ steps.rust-toolchain.outputs.cachekey }}-
+            ${{ runner.os }}-cargo-
+      - run: cargo rustdoc --all-features -- --cfg docsrs
       - run: chmod -c -R +rX "target/doc"
       - run: echo "<meta http-equiv=refresh content=0;url=substrait>" > target/doc/index.html
       - if: github.event_name == 'push' && github.ref == 'refs/heads/main'

diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml
@@ -18,11 +18,22 @@ jobs:
           fetch-depth: 0
           submodules: recursive
       - uses: dtolnay/rust-toolchain@stable
-      - run: cargo install cargo-smart-release --debug --locked
-      - uses: arduino/setup-protoc@v3
+        id: rust-toolchain
+      - uses: actions/cache@v4
         with:
-          repo-token: ${{ secrets.GITHUB_TOKEN }}
-      - run: cargo check
+          path: |
+            ~/.cargo/bin/
+            ~/.cargo/registry/index/
+            ~/.cargo/registry/cache/
+            ~/.cargo/git/db/
+            target/
+          key: ${{ runner.os }}-cargo-${{ steps.rust-toolchain.outputs.cachekey }}-release-${{ hashFiles('**/Cargo.toml') }}
+          restore-keys: |
+            ${{ runner.os }}-cargo-${{ steps.rust-toolchain.outputs.cachekey }}-release-
+            ${{ runner.os }}-cargo-${{ steps.rust-toolchain.outputs.cachekey }}-
+            ${{ runner.os }}-cargo-
+      - run: cargo install cargo-smart-release --debug --locked
+      - run: cargo check --all-features
       - run: cargo changelog --execute substrait
       - run: |
           git config user.name github-actions[bot]

diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -8,46 +8,53 @@ jobs:
   check:
     name: Check
     runs-on: ubuntu-latest
-    strategy:
-      fail-fast: false
-      matrix:
-        feature:
-          - default
-          - pbjson
-          - protoc
-          - semver,serde
     steps:
       - uses: actions/checkout@v4
         with:
           fetch-depth: 0
           submodules: recursive
       - uses: dtolnay/rust-toolchain@stable
-      - uses: arduino/setup-protoc@v3
-        if: matrix.feature != 'protoc'
+        id: rust-toolchain
+      - uses: actions/cache@v4
         with:
-          repo-token: ${{ secrets.GITHUB_TOKEN }}
-      - run: cargo check --features ${{ matrix.feature }}
+          path: |
+            ~/.cargo/bin/
+            ~/.cargo/registry/index/
+            ~/.cargo/registry/cache/
+            ~/.cargo/git/db/
+            target/
+          key: ${{ runner.os }}-cargo-${{ steps.rust-toolchain.outputs.cachekey }}-check-${{ hashFiles('**/Cargo.toml') }}
+          restore-keys: |
+            ${{ runner.os }}-cargo-${{ steps.rust-toolchain.outputs.cachekey }}-check-
+            ${{ runner.os }}-cargo-${{ steps.rust-toolchain.outputs.cachekey }}-
+            ${{ runner.os }}-cargo-
+      - run: cargo check --all-targets --all-features
 
   test:
     name: Test
     runs-on: ubuntu-latest
-    strategy:
-      fail-fast: false
-      matrix:
-        feature:
-          - default
-          - pbjson
-          - semver,serde
     steps:
       - uses: actions/checkout@v4
         with:
           fetch-depth: 0
           submodules: recursive
       - uses: dtolnay/rust-toolchain@stable
-      - uses: arduino/setup-protoc@v3
+        id: rust-toolchain
+      - uses: actions/cache@v4
         with:
-          repo-token: ${{ secrets.GITHUB_TOKEN }}
-      - run: cargo test --features ${{ matrix.feature }}
+          path: |
+            ~/.cargo/bin/
+            ~/.cargo/registry/index/
+            ~/.cargo/registry/cache/
+            ~/.cargo/git/db/
+            target/
+          key: ${{ runner.os }}-cargo-${{ steps.rust-toolchain.outputs.cachekey }}-test-${{ hashFiles('**/Cargo.toml') }}
+          restore-keys: |
+            ${{ runner.os }}-cargo-${{ steps.rust-toolchain.outputs.cachekey }}-test-
+            ${{ runner.os }}-cargo-${{ steps.rust-toolchain.outputs.cachekey }}-
+            ${{ runner.os }}-cargo-
+      - run: cargo test --all-targets --all-features
+      - run: cargo test --doc --all-features
 
   rustfmt:
     name: Rustfmt
@@ -62,25 +69,29 @@ jobs:
   clippy:
     name: Clippy
     runs-on: ubuntu-latest
-    strategy:
-      fail-fast: false
-      matrix:
-        feature:
-          - default
-          - pbjson
-          - semver,serde
     steps:
       - uses: actions/checkout@v4
         with:
           fetch-depth: 0
           submodules: recursive
       - uses: dtolnay/rust-toolchain@stable
+        id: rust-toolchain
         with:
           components: clippy
-      - uses: arduino/setup-protoc@v3
+      - uses: actions/cache@v4
         with:
-          repo-token: ${{ secrets.GITHUB_TOKEN }}
-      - run: cargo clippy --features ${{ matrix.feature }} -- -Dwarnings
+          path: |
+            ~/.cargo/bin/
+            ~/.cargo/registry/index/
+            ~/.cargo/registry/cache/
+            ~/.cargo/git/db/
+            target/
+          key: ${{ runner.os }}-cargo-${{ steps.rust-toolchain.outputs.cachekey }}-clippy-${{ hashFiles('**/Cargo.toml') }}
+          restore-keys: |
+            ${{ runner.os }}-cargo-${{ steps.rust-toolchain.outputs.cachekey }}-clippy-
+            ${{ runner.os }}-cargo-${{ steps.rust-toolchain.outputs.cachekey }}-
+            ${{ runner.os }}-cargo-
+      - run: cargo clippy --all-targets --all-features -- -Dwarnings
 
   package:
     name: Package
@@ -91,8 +102,19 @@ jobs:
           fetch-depth: 0
           submodules: recursive
       - uses: dtolnay/rust-toolchain@stable
-      - uses: arduino/setup-protoc@v3
+        id: rust-toolchain
+      - uses: actions/cache@v4
         with:
-          repo-token: ${{ secrets.GITHUB_TOKEN }}
-      - run: cargo build
-      - run: cargo package --allow-dirty
+          path: |
+            ~/.cargo/bin/
+            ~/.cargo/registry/index/
+            ~/.cargo/registry/cache/
+            ~/.cargo/git/db/
+            target/
+          key: ${{ runner.os }}-cargo-${{ steps.rust-toolchain.outputs.cachekey }}-package-${{ hashFiles('**/Cargo.toml') }}
+          restore-keys: |
+            ${{ runner.os }}-cargo-${{ steps.rust-toolchain.outputs.cachekey }}-package-
+            ${{ runner.os }}-cargo-${{ steps.rust-toolchain.outputs.cachekey }}-
+            ${{ runner.os }}-cargo-
+      - run: cargo build --all-features
+      - run: cargo package --all-features --allow-dirty
diff --git a/Cargo.toml b/Cargo.toml
@@ -13,55 +13,50 @@ license = "Apache-2.0"
 keywords = ["substrait"]
 build = "build.rs"
 include = [
-  "LICENSE",
-  "build.rs",
-  "src/**/*.rs",
-  "src/version.in",
-  "substrait/LICENSE",
-  "substrait/README.md",
-  "substrait/extensions/**/*.yaml",
-  "substrait/proto/**/*.proto",
-  "substrait/text/**/*.yaml",
+    "LICENSE",
+    "build.rs",
+    "src/**/*.rs",
+    "src/version.in",
+    "substrait/LICENSE",
+    "substrait/README.md",
+    "substrait/extensions/**/*.yaml",
+    "substrait/proto/**/*.proto",
+    "substrait/text/**/*.yaml",
 ]
 
 [features]
 default = []
-pbjson = ["dep:pbjson", "dep:pbjson-build", "dep:pbjson-types"]
 protoc = ["dep:protobuf-src"]
-serde = ["dep:prost-wkt", "dep:prost-wkt-build", "dep:prost-wkt-types"]
 semver = ["dep:semver"]
+serde = ["dep:pbjson", "dep:pbjson-build", "dep:pbjson-types"]
 
 [dependencies]
 pbjson = { version = "0.6.0", optional = true }
 pbjson-types = { version = "0.6.0", optional = true }
-prost = "0.12"
-prost-types = "0.12"
-prost-wkt = { version = "0.5", optional = true }
-prost-wkt-types = { version = "0.5", optional = true }
-semver = { version = "1", optional = true }
-serde = { version = "1", features = ["derive"] }
-serde_json = "1"
+prost = "0.12.3"
+prost-types = "0.12.3"
+semver = { version = "1.0.22", optional = true }
+serde = { version = "1.0.197", features = ["derive"] }
+serde_json = "1.0.114"
 
 [build-dependencies]
 git2 = { version = "0.18.2", default-features = false }
-heck = "0.4"
+heck = "0.4.1"
 pbjson-build = { version = "0.6.2", optional = true }
 prettyplease = "0.2.4"
-prost-build = { version = "0.12", default-features = false }
-prost-types = "0.12"
-prost-wkt-build = { version = "0.5", optional = true }
-protobuf-src = { version = "1", optional = true }
-schemars = "0.8"
-semver = "1"
-serde_yaml = "0.9"
+prost-build = { version = "0.12.3", default-features = false }
+protobuf-src = { version = "1.1.0", optional = true }
+schemars = "0.8.16"
+semver = "1.0.22"
+serde_yaml = "0.9.32"
 syn = "2.0.11"
 typify = "0.0.16"
-walkdir = "2"
+walkdir = "2.5.0"
 
 [dev-dependencies]
-serde_yaml = "0.9"
-walkdir = "2"
+serde_yaml = "0.9.32"
+walkdir = "2.5.0"
 
 [package.metadata.docs.rs]
-features = ["semver"]
+all-features = true
 rustdoc-args = ["--cfg", "docsrs"]
diff --git a/build.rs b/build.rs
@@ -13,9 +13,6 @@ use walkdir::{DirEntry, WalkDir};
 const PROTO_ROOT: &str = "substrait/proto";
 const TEXT_ROOT: &str = "substrait/text";
 
-#[cfg(all(feature = "serde", feature = "pbjson"))]
-compile_error!("Either feature `serde` or `pbjson` can be enabled");
-
 /// Add Substrait version information to the build
 fn substrait_version() -> Result<(), Box<dyn Error>> {
     use git2::{DescribeFormatOptions, DescribeOptions, Repository};
@@ -84,7 +81,7 @@ pub const SUBSTRAIT_GIT_SHA: &str = "{git_hash}";
 /// crate
 pub const SUBSTRAIT_GIT_DESCRIBE: &str = "{git_describe}";
 
-/// The amount of commits between the latest tag and this version of the 
+/// The amount of commits between the latest tag and this version of the
 /// Substrait module used to build this crate
 pub const SUBSTRAIT_GIT_DEPTH: u32 = {git_depth};
 
@@ -168,49 +165,8 @@ pub mod {title} {{
 }
 
 #[cfg(feature = "serde")]
-/// Serialize deserialize implementations for proto types using `serde`
-fn serde(protos: &[impl AsRef<Path>], out_dir: PathBuf) -> Result<(), Box<dyn Error>> {
-    use prost_types::DescriptorProto;
-    use prost_wkt_build::{FileDescriptorSet, Message};
-
-    let descriptor_path = out_dir.join("proto_descriptor.bin");
-
-    fn serde_default(cfg: &mut Config, dp: Vec<DescriptorProto>, path: String) {
-        dp.into_iter().for_each(move |descriptor| {
-            let name = descriptor.name().to_string();
-            cfg.type_attribute(format!("{path}.{name}"), "#[serde(default)]");
-            serde_default(cfg, descriptor.nested_type, format!("{path}.{name}"))
-        });
-    }
-
-    let mut cfg = Config::new();
-    cfg.file_descriptor_set_path(&descriptor_path)
-        .type_attribute(".", "#[derive(serde::Deserialize, serde::Serialize)]")
-        .extern_path(".google.protobuf.Any", "::prost_wkt_types::Any")
-        .compile_protos(protos, &[PROTO_ROOT])?;
-
-    FileDescriptorSet::decode(&mut fs::read(&descriptor_path)?.as_slice())?
-        .file
-        .into_iter()
-        .for_each(|fdp| {
-            let package = fdp.package().into();
-            serde_default(&mut cfg, fdp.message_type, package)
-        });
-
-    cfg.skip_protoc_run()
-        .compile_protos(protos, &[PROTO_ROOT])?;
-
-    prost_wkt_build::add_serde(
-        out_dir,
-        FileDescriptorSet::decode(fs::read(descriptor_path)?.as_slice())?,
-    );
-
-    Ok(())
-}
-
-#[cfg(feature = "pbjson")]
 /// Serialize and deserialize implementations for proto types using `pbjson`
-fn pbjson(protos: &[impl AsRef<Path>], out_dir: PathBuf) -> Result<(), Box<dyn Error>> {
+fn serde(protos: &[impl AsRef<Path>], out_dir: PathBuf) -> Result<(), Box<dyn Error>> {
     use pbjson_build::Builder;
 
     let descriptor_path = out_dir.join("proto_descriptor.bin");
@@ -257,13 +213,10 @@ fn main() -> Result<(), Box<dyn Error>> {
         })
         .collect::<Vec<_>>();
 
-    #[cfg(feature = "pbjson")]
-    pbjson(&protos, out_dir)?;
-
     #[cfg(feature = "serde")]
     serde(&protos, out_dir)?;
 
-    #[cfg(not(any(feature = "serde", feature = "pbjson")))]
+    #[cfg(not(feature = "serde"))]
     Config::new().compile_protos(&protos, &[PROTO_ROOT])?;
 
     Ok(())