feat: bump datafusion to 48

[colinmarc]

This did not require very many changes!

[colinmarc]

Famous last words! The CI issues seem to mostly revolve around everything needing protoc to build now (because of datafusion-substrait).

[Xuanwo]

Famous last words! The CI issues seem to mostly revolve around everything needing protoc to build now (because of datafusion-substrait).

Is there an issue for this? I don't know on the current status or the reason we're blocked.

[kevinjqliu]

summarizing the slack thread:

we are currently blocked due to MSRV versions.

Datafusion 48 requires aws-config 1.6.2 which requires rust 1.86.
We're currently using rust 1.85.

Typically we bump the MSRV for a new release of iceberg-rust, https://github.com/apache/iceberg-rust?tab=readme-ov-file#supported-rust-version
I started the PR to upgrade #1655

[kevinjqliu]

#1655 is merged, should unblock this. @colinmarc could you try rebasing main?

[colinmarc]

It's currently failing to install protoc on the mac builds:

Error: API rate limit exceeded for 13.105.117.136. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)

[kevinjqliu]

oh yea the fix for rate limiting is to include the token. the token is automatically set up in CI

      - name: Install Protoc
        uses: arduino/setup-protoc@v3
        with:
          repo-token: ${{ secrets.GITHUB_TOKEN }}

[colinmarc]

Hm, among other things, the MSRV check is still failing.

[kevinjqliu]

Hm, among other things, the MSRV check is still failing.

perhaps an issue related to #1656

[kevinjqliu]

I tried regenerating the Cargo.lock file, lets see if this will resolve CI :)

[kevinjqliu]

woot! finally!

LGTM

[liurenjie1024]

Thanks @colinmarc for this pr. In general this looks good to me, but I assume that the make test would fail due to the dependency of protobuf? If this is the case, I would suggest to also add a step in our make file to fix it?

[kevinjqliu]

looks like the protoc requirement is from
datafusion-sqllogictest -> datafusion-substrait -> substrait

Heres the original error message:

error: failed to run custom build command for `substrait v0.56.0`

Caused by:
  process didn't exit successfully: `/home/runner/work/iceberg-rust/iceberg-rust/target/debug/build/substrait-9f341031683ef1a7/build-script-build` (exit status: 1)
  --- stdout
  cargo:rerun-if-env-changed=FORCE_REBUILD
  cargo:rerun-if-changed=substrait
  cargo:rerun-if-changed=substrait/text/simple_extensions_schema.yaml
  cargo:rerun-if-changed=substrait/proto/substrait/type.proto
  cargo:rerun-if-changed=substrait/proto/substrait/extensions/extensions.proto
  cargo:rerun-if-changed=substrait/proto/substrait/extended_expression.proto
  cargo:rerun-if-changed=substrait/proto/substrait/algebra.proto
  cargo:rerun-if-changed=substrait/proto/substrait/plan.proto
  cargo:rerun-if-changed=substrait/proto/substrait/function.proto
  cargo:rerun-if-changed=substrait/proto/substrait/parameterized_types.proto
  cargo:rerun-if-changed=substrait/proto/substrait/type_expressions.proto
  cargo:rerun-if-changed=substrait/proto/substrait/capabilities.proto

  --- stderr
  Error: Custom { kind: NotFound, error: "Could not find `protoc`. If `protoc` is installed, try setting the `PROTOC` environment variable to the path of the `protoc` binary. To install it on Debian, run `apt-get install protobuf-compiler`. It is also available at https://github.com/protocolbuffers/protobuf/releases  For more information: https://docs.rs/prost-build/#sourcing-protoc" }
warning: build failed, waiting for other jobs to finish...
make: *** [Makefile:21: build] Error 101

If this is the case, I would suggest to also add a step in our make file to fix it?

I uninstalled protoc locally with brew uninstall protobuf and verify that make build fails locally.

[kevinjqliu]

we can potentially do something like this

https://github.com/guacsec/guac/blob/3aa915a571f2c3a097aa236144eb990b46a7fbb6/Makefile#L275-L281

[liurenjie1024]

we can potentially do something like this

https://github.com/guacsec/guac/blob/3aa915a571f2c3a097aa236144eb990b46a7fbb6/Makefile#L275-L281

Yes. Installing protobuf is too aggressive. Adding an error message would be enough.

[colinmarc]

Adding an error message would be enough.

There's already an error message that I think is pretty clear, and enforcing protoc on our side seems brittle (substrait could remove the requirement in a future release, for instance). What's the reason to add another one?

[kevinjqliu]

I think the current error message from substrait is pretty informative already

Error: Custom { kind: NotFound, error: "Could not find `protoc`. If `protoc` is installed, try setting the `PROTOC` environment variable to the path of the `protoc` binary. To install it on Debian, run `apt-get install protobuf-compiler`. It is also available at https://github.com/protocolbuffers/protobuf/releases  For more information: https://docs.rs/prost-build/#sourcing-protoc" }

I think we can merge this PR for now, and follow up with another PR to add protoc check in the Makefile if necessary

[kevinjqliu]

Thank you for the PR @colinmarc and thanks for the review @liurenjie1024 @Fokko

[liurenjie1024]

Error: Custom { kind: NotFound, error: "Could not find `protoc`. If `protoc` is installed, try setting the `PROTOC` environment variable to the path of the `protoc` binary. To install it on Debian, run `apt-get install protobuf-compiler`. It is also available at https://github.com/protocolbuffers/protobuf/releases  For more information: https://docs.rs/prost-build/#sourcing-protoc" }

+1, my suggestion should not be a blocker.