apache / apache/sedona-db

rust/sedona-pointcloud: Improve integration test coverage for las and laz readers

Open
#834 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
503
Forks
61
Avg merge
2d 4h
Merged PRs (30d)
90

Description

A couple of suggestions before merging:

**Tests**
- The previous `projection` test covered both `.las` and `.laz`; the rewrite only covers `.las`. Since LAZ goes through a separate decode path, it'd be good to keep the `.laz` assertion (or parameterize over both).
- Assertions check column count + type of column 0, but not the actual data. The interesting failure mode in #824 was returning the **right schema with the wrong column** — same UInt8 shape, wrong values. An `assert_eq!` on a value (e.g. `classification = 0` for the single point in `extra.las`) would lock that down.

**Scope**
- `LasSource::new(extension, table_schema: impl Into)` is an unrelated public-API change. Worth pulling out into a separate PR (or dropping) so this one is purely a fix.
- The field reordering in `LasSource` and `LasOpener` is noisy in the diff without changing behaviour; reverting it would make the fix easier to cherry-pick.

_Originally posted by @jiayuasu in https://github.com/apache/sedona-db/issues/825#issuecomment-4417226182_

Contributor guide

Open the contributing guide

Research direction

Start with the rewritten projection integration test and the LasSource and LasOpener readers. Run the test with both extra.las and its .laz counterpart, then add an assertion for the returned classification value and keep the unrelated public-API and field-order changes out of scope.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
database, testing
Issue type
Refactor
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.