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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
