jiayuasu commented on PR #825: URL: https://github.com/apache/sedona-db/pull/825#issuecomment-4417226182
Approach looks right — composing via `try_merge` and applying `file_indices` in the opener nails both halves of #824. Confirmed locally that the new test query exercises the repeated-pushdown path (two `try_pushdown_projection` invocations, the second with indices into the projected output), so the `try_merge` branch is covered. 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<TableSchema>)` 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. Happy to push a follow-up commit if useful. -- 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]
