You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@sedona.apache.org by "paleolimbot (via GitHub)" <gi...@apache.org> on 2026/09/02 22:31:42 UTC
[PR] build(rust): upgrade DataFusion to 54.1.0 [sedona-db]
paleolimbot opened a new pull request, #1226:
URL: https://github.com/apache/sedona-db/pull/1226
Upgrades DataFusion from 52.5.0 to 54.1.0 with the corresponding Arrow, Parquet, object_store, GeoArrow, and Zarr versions. Includes initial trait, downcast, execution-plan, and metadata-cache API migrations.
Draft blockers: remaining compilation fixes are concentrated in datasource, GeoParquet, and query-planner APIs.
Decision needed: DataFusion removed registry-driven protobuf expression decoding. Session-backed decoding can use a synthesized TaskContext, but preserving dynamic placeholder UDFs requires either pre-scanning protobuf expressions or proposing an upstream registry hook.
Testing: cargo fmt --all; cargo check --workspace --lib (currently fails at the draft blockers above).
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore: upgrade DataFusion to 54.1.0 [sedona-db]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on code in PR #1226:
URL: https://github.com/apache/sedona-db/pull/1226#discussion_r3975737169
##########
rust/sedona/src/context.rs:
##########
@@ -255,31 +265,57 @@ impl SedonaContext {
// `enable_url_table` so directory-shaped external formats (Zarr)
// resolve to a single-object table rather than a listing over the
// directory's contents.
- let ctx = enable_sedona_url_table(SessionContext::new_with_state(state));
+ let ctx = SessionContext::new_with_state(state);
- // Install dynamic catalog provider that can register required object stores
+ // Load the configured catalogs before rebuilding the context below.
ctx.refresh_catalogs().await?;
- ctx.register_catalog_list(Arc::new(DynamicObjectStoreCatalog::new(
- ctx.state().catalog_list().clone(),
- ctx.state_weak_ref(),
- )));
- Self::new_from_context(ctx)
+ let out = Self::new_from_context(ctx)?;
+
+ // Install state-dependent catalog wrappers after `new_from_context()`
+ // rebuilds the SessionContext so their weak references point at the
+ // live SessionState.
+ install_sedona_url_table(&out.ctx);
+
+ // Install the dynamic catalog provider only after `new_from_context()`
+ // rebuilds the SessionContext. The provider stores a weak reference to
+ // SessionState; installing it before the rebuild leaves it pointing at
+ // the dropped state and causes catalog misses to report a locking error.
+ out.ctx
+ .register_catalog_list(Arc::new(DynamicObjectStoreCatalog::new(
+ out.ctx.state().catalog_list().clone(),
+ out.ctx.state_weak_ref(),
+ )));
+
+ Ok(out)
}
/// Creates a new context from a previously configured DataFusion context
pub fn new_from_context(ctx: SessionContext) -> Result<Self> {
+ // DataFusion #22620 workaround tracked by
+ // https://github.com/apache/sedona-db/issues/1232.
+ let state_builder = register_vendored_optimizer_rules(ctx.into_state_builder())?;
+ let ctx = SessionContext::new_with_state(state_builder.build());
Review Comment:
Could we keep the existing session state here? Rebuilding it leaves the URL-table catalog pointing to the old state, which has been dropped. Moving the catalog setup fixes the interactive constructor, but a caller-supplied context still breaks.
Repro: save as `rust/sedona/tests/supplied_url_table.rs`, then run `cargo test -p sedona --test supplied_url_table`. The query succeeds before wrapping and fails afterward with `get current SessionStore error`.
```rust
#[tokio::test]
async fn supplied_url_table() -> datafusion::error::Result<()> {
use datafusion::prelude::SessionContext;
use sedona::context::SedonaContext;
let dir = tempfile::tempdir()?;
let path = dir.path().join("example.csv");
std::fs::write(&path, "x\n1\n2\n")?;
let sql = format!("SELECT * FROM '{}'", path.display());
let ctx = SessionContext::new().enable_url_table();
ctx.sql(&sql).await?.collect().await?; // succeeds
let ctx = SedonaContext::new_from_context(ctx)?;
ctx.ctx.sql(&sql).await?.collect().await?; // fails
Ok(())
}
```
##########
rust/sedona/src/context.rs:
##########
@@ -255,31 +265,57 @@ impl SedonaContext {
// `enable_url_table` so directory-shaped external formats (Zarr)
// resolve to a single-object table rather than a listing over the
// directory's contents.
- let ctx = enable_sedona_url_table(SessionContext::new_with_state(state));
+ let ctx = SessionContext::new_with_state(state);
- // Install dynamic catalog provider that can register required object stores
+ // Load the configured catalogs before rebuilding the context below.
ctx.refresh_catalogs().await?;
- ctx.register_catalog_list(Arc::new(DynamicObjectStoreCatalog::new(
- ctx.state().catalog_list().clone(),
- ctx.state_weak_ref(),
- )));
- Self::new_from_context(ctx)
+ let out = Self::new_from_context(ctx)?;
+
+ // Install state-dependent catalog wrappers after `new_from_context()`
+ // rebuilds the SessionContext so their weak references point at the
+ // live SessionState.
+ install_sedona_url_table(&out.ctx);
+
+ // Install the dynamic catalog provider only after `new_from_context()`
+ // rebuilds the SessionContext. The provider stores a weak reference to
+ // SessionState; installing it before the rebuild leaves it pointing at
+ // the dropped state and causes catalog misses to report a locking error.
+ out.ctx
+ .register_catalog_list(Arc::new(DynamicObjectStoreCatalog::new(
+ out.ctx.state().catalog_list().clone(),
+ out.ctx.state_weak_ref(),
+ )));
+
+ Ok(out)
}
/// Creates a new context from a previously configured DataFusion context
pub fn new_from_context(ctx: SessionContext) -> Result<Self> {
+ // DataFusion #22620 workaround tracked by
+ // https://github.com/apache/sedona-db/issues/1232.
+ let state_builder = register_vendored_optimizer_rules(ctx.into_state_builder())?;
Review Comment:
Could this accept a context where the caller has already disabled the rule? There is nothing to replace in that case, but construction now fails with `PushDownLeafProjections rule not found in default optimizer rules`.
Repro: save as `rust/sedona/tests/disabled_leaf_projection.rs`, then run `cargo test -p sedona --test disabled_leaf_projection`.
```rust
#[tokio::test]
async fn disabled_leaf_projection() -> datafusion::error::Result<()> {
use datafusion::prelude::SessionContext;
use sedona::context::SedonaContext;
let ctx = SessionContext::new();
assert!(ctx.remove_optimizer_rule("push_down_leaf_projections"));
SedonaContext::new_from_context(ctx)?;
Ok(())
}
```
##########
rust/sedona-pointcloud/src/las/metadata.rs:
##########
@@ -379,6 +394,10 @@ async fn laz_chunk_table(
let laz_vlr = header.laz_vlr()?;
let num_points = header.number_of_points();
+ if num_points == 0 {
+ return Ok(Vec::new());
Review Comment:
Could we skip or regenerate the old statistics when the file has no points? The previous reader gave an empty format-0 LAZ one zero-point chunk and persisted one statistics row. Returning no chunks here makes `chunk_statistics()` panic when it reuses that sidecar: `statistics.rs:232`, `left: 1, right: 0`.
Repro: save this as `rust/sedona-pointcloud/tests/empty_laz_sidecar.rs` in both checkouts. On `69c8a4c0`, run:
```sh
export EMPTY_LAZ_PROBE="$(mktemp -d)/empty.laz"
cargo test -p sedona-pointcloud --test empty_laz_sidecar
```
Then run the same Cargo command on `10335895` in the same shell, keeping the LAZ file and its `.stats` sidecar. The old reader succeeds; the new reader panics.
```rust
#[tokio::test]
async fn empty_laz_statistics_reuse() {
use las::{point::Format, Builder, Writer};
use object_store::{local::LocalFileSystem, path::Path, ObjectStoreExt};
use sedona_pointcloud::las::{metadata::LasMetadataReader, options::LasOptions};
let path = std::path::PathBuf::from(std::env::var("EMPTY_LAZ_PROBE").unwrap());
if !path.exists() {
let mut builder = Builder::from((1, 4));
builder.point_format = Format::new(0).unwrap();
builder.point_format.is_compressed = true;
let mut writer = Writer::from_path(&path, builder.into_header().unwrap()).unwrap();
writer.close().unwrap();
}
let store = LocalFileSystem::new();
let location = Path::from_filesystem_path(&path).unwrap();
let object = store.head(&location).await.unwrap();
let mut options = LasOptions::default();
options.collect_statistics = true;
options.persist_statistics = true;
LasMetadataReader::new(&store, &object)
.with_options(options)
.fetch_metadata()
.await
.unwrap();
}
```
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore: upgrade DataFusion to 54.1.0 [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #1226:
URL: https://github.com/apache/sedona-db/pull/1226#issuecomment-5622400478
@jiayuasu Any further comments here?
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore: upgrade DataFusion to 54.1.0 [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #1226:
URL: https://github.com/apache/sedona-db/pull/1226#issuecomment-5546179093
@zhangfengcdt Can you take a look at this one...I think it's ready for 👀 !
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore: upgrade DataFusion to 54.1.0 [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #1226:
URL: https://github.com/apache/sedona-db/pull/1226#issuecomment-5624048321
Merging because this is a big change that I'd like to base other work on without conflict...happy to solve any outstanding issues as a follow up.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore: upgrade DataFusion to 54.1.0 [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #1226:
URL: https://github.com/apache/sedona-db/pull/1226#discussion_r3980721300
##########
rust/sedona/src/context.rs:
##########
@@ -255,31 +265,57 @@ impl SedonaContext {
// `enable_url_table` so directory-shaped external formats (Zarr)
// resolve to a single-object table rather than a listing over the
// directory's contents.
- let ctx = enable_sedona_url_table(SessionContext::new_with_state(state));
+ let ctx = SessionContext::new_with_state(state);
- // Install dynamic catalog provider that can register required object stores
+ // Load the configured catalogs before rebuilding the context below.
ctx.refresh_catalogs().await?;
- ctx.register_catalog_list(Arc::new(DynamicObjectStoreCatalog::new(
- ctx.state().catalog_list().clone(),
- ctx.state_weak_ref(),
- )));
- Self::new_from_context(ctx)
+ let out = Self::new_from_context(ctx)?;
+
+ // Install state-dependent catalog wrappers after `new_from_context()`
+ // rebuilds the SessionContext so their weak references point at the
+ // live SessionState.
+ install_sedona_url_table(&out.ctx);
+
+ // Install the dynamic catalog provider only after `new_from_context()`
+ // rebuilds the SessionContext. The provider stores a weak reference to
+ // SessionState; installing it before the rebuild leaves it pointing at
+ // the dropped state and causes catalog misses to report a locking error.
+ out.ctx
+ .register_catalog_list(Arc::new(DynamicObjectStoreCatalog::new(
+ out.ctx.state().catalog_list().clone(),
+ out.ctx.state_weak_ref(),
+ )));
+
+ Ok(out)
}
/// Creates a new context from a previously configured DataFusion context
pub fn new_from_context(ctx: SessionContext) -> Result<Self> {
+ // DataFusion #22620 workaround tracked by
+ // https://github.com/apache/sedona-db/issues/1232.
+ let state_builder = register_vendored_optimizer_rules(ctx.into_state_builder())?;
+ let ctx = SessionContext::new_with_state(state_builder.build());
Review Comment:
I just made new_from_context private given the complexity of what we need to do to construct it at this point.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore: upgrade DataFusion to 54.1.0 [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #1226:
URL: https://github.com/apache/sedona-db/pull/1226#discussion_r3980729025
##########
rust/sedona-pointcloud/src/las/metadata.rs:
##########
@@ -379,6 +394,10 @@ async fn laz_chunk_table(
let laz_vlr = header.laz_vlr()?;
let num_points = header.number_of_points();
+ if num_points == 0 {
+ return Ok(Vec::new());
Review Comment:
I fixed this but also updated the arithmetic to use checked variants so the failure mode of a corrupted sidecar is not a panic!
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore: upgrade DataFusion to 54.1.0 [sedona-db]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on code in PR #1226:
URL: https://github.com/apache/sedona-db/pull/1226#discussion_r3939746493
##########
rust/sedona-pointcloud/src/las/statistics.rs:
##########
@@ -33,7 +33,7 @@ use byteorder::{LittleEndian, ReadBytesExt};
use datafusion_common::{arrow::compute::concat_batches, Column, DataFusionError, ScalarValue};
use datafusion_pruning::PruningStatistics;
use las::Header;
-use object_store::{path::Path, ObjectMeta, ObjectStore, PutPayload};
+use object_store::{path::Path, ObjectMeta, ObjectStore, ObjectStoreExt, PutPayload};
Review Comment:
Could we add the zero-point guard to `extract_chunk_stats()` too? An empty LAZ file using point format 6 has a `477..477` chunk. Normal reading now works, but `collect_statistics=true` still fails with `Requested range was invalid`. The old object_store 0.12.4 accepted that empty range.
Repro: save as `rust/sedona-pointcloud/tests/empty_laz_statistics.rs`, then run `cargo test -p sedona-pointcloud --test empty_laz_statistics`. The final call fails; setting `collect_statistics` to `false` succeeds.
```rust
#[tokio::test]
async fn empty_laz_statistics() {
use las::{point::Format, Builder, Writer};
use object_store::{local::LocalFileSystem, path::Path, ObjectStoreExt};
use sedona_pointcloud::las::{metadata::LasMetadataReader, options::LasOptions};
let dir = tempfile::tempdir().unwrap();
let path = dir.path().join("empty.laz");
let mut builder = Builder::from((1, 4));
builder.point_format = Format::new(6).unwrap();
builder.point_format.is_compressed = true;
let mut writer = Writer::from_path(&path, builder.into_header().unwrap()).unwrap();
writer.close().unwrap();
let store = LocalFileSystem::new();
let location = Path::from_filesystem_path(&path).unwrap();
let object = store.head(&location).await.unwrap();
let metadata = LasMetadataReader::new(&store, &object)
.fetch_metadata().await.unwrap();
assert_eq!(metadata.header.number_of_points(), 0);
let mut options = LasOptions::default();
options.collect_statistics = true;
LasMetadataReader::new(&store, &object)
.with_options(options)
.fetch_metadata()
.await
.unwrap();
}
```
##########
rust/sedona/src/context.rs:
##########
@@ -100,7 +103,11 @@ impl SedonaContext {
pub fn new() -> Self {
// This will panic only if the default build settings are
// incorrect which we test!
- Self::new_from_context(SessionContext::new()).unwrap()
+ let state_builder = SessionStateBuilder::new_with_default_features();
+ // DataFusion #22620 workaround tracked by
+ // https://github.com/apache/sedona-db/issues/1232.
+ let state_builder = register_vendored_optimizer_rules(state_builder).unwrap();
Review Comment:
Could this also be applied in `new_from_context()`? Passing a regular `SessionContext` still hits the Unnest bug. This test fails in `push_down_leaf_projections`; changing the constructor to `SedonaContext::new()` returns the expected two rows.
Repro: save as `rust/sedona/tests/supplied_context_dump.rs`, then run `cargo test -p sedona --test supplied_context_dump`.
```rust
#[tokio::test]
async fn supplied_context_dump() -> datafusion::error::Result<()> {
use datafusion::{
functions::core::expr_fn::get_field,
prelude::{col, SessionContext},
};
use sedona::context::SedonaContext;
let ctx = SedonaContext::new_from_context(SessionContext::new())?;
let batches = ctx
.sql("SELECT ST_Dump(ST_GeomFromText('MULTIPOINT (0 0, 1 1)')) AS dump")
.await?
.unnest_columns(&["dump"])?
.select(vec![get_field(col("dump"), "geom").alias("geometry")])?
.collect()
.await?;
assert_eq!(batches.iter().map(|b| b.num_rows()).sum::<usize>(), 2);
Ok(())
}
```
##########
examples/sedonadb-rust/Cargo.lock:
##########
@@ -4414,26 +5390,28 @@ dependencies = [
[[package]]
name = "sedona"
-version = "0.4.0"
-source = "git+https://github.com/apache/sedona-db.git#9d6bcfa178a341e23f40d58a7ec6244f7b5dca25"
+version = "0.5.0"
+source = "git+https://github.com/apache/sedona-db.git#7b2864a996cd14edde6bb2c8d451ca5df52756fe"
Review Comment:
This revision still uses DataFusion 52.5, while the example now depends on 54.1. Could we regenerate the lockfile against a Sedona revision containing the upgrade?
From this PR checkout:
```sh
cd examples/sedonadb-rust
cargo tree --locked --depth 1 -p sedonadb-rust-example
cargo tree --locked --depth 1 -p sedona
cargo check --locked
```
The first tree shows DataFusion 54.1; the second shows 52.5. The example then passes a 54.1 `Expr` from `col("name")` to `sort_by`, which expects a 52.5 `Expr` (`E0308`). The `?` calls also have incompatible `DataFusionError` types. CI rewrites the Sedona dependency to the PR head before building, so it does not test this committed lockfile.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore: upgrade DataFusion to 54.1.0 [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #1226:
URL: https://github.com/apache/sedona-db/pull/1226
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org