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/05/01 18:22:49 UTC
[PR] feat(rust/sedona-geoparquet): Add GeoParquet 2.0/Parquet-native geometry and geography support to parque writer [sedona-db]
paleolimbot opened a new pull request, #805:
URL: https://github.com/apache/sedona-db/pull/805
(no comment)
--
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] feat(rust/sedona-geoparquet): Add GeoParquet 2.0/Parquet-native geometry and geography support to parque writer [sedona-db]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #805:
URL: https://github.com/apache/sedona-db/pull/805#discussion_r3192592001
##########
rust/sedona-geoparquet/src/writer.rs:
##########
@@ -532,8 +574,184 @@ fn append_float_bbox(
Ok(())
}
+#[derive(Debug, PartialEq)]
+struct NormalizeForGeoParquet {
+ crs_provider: CrsProviderOption,
+ version: GeoParquetVersion,
+ signature: Signature,
+}
+
+impl NormalizeForGeoParquet {
+ fn new(crs_provider: CrsProviderOption, version: GeoParquetVersion) -> Self {
+ Self {
+ crs_provider,
+ version,
+ signature: Signature::any(1, Volatility::Stable),
+ }
+ }
+}
+
+impl std::hash::Hash for NormalizeForGeoParquet {
+ fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+ self.version.hash(state);
+ self.signature.hash(state);
+ }
+}
+
+impl Eq for NormalizeForGeoParquet {}
+
+impl ScalarUDFImpl for NormalizeForGeoParquet {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn name(&self) -> &str {
+ "normalize_for_geoparquet"
+ }
+
+ fn signature(&self) -> &Signature {
+ &self.signature
+ }
+
+ fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
+ sedona_internal_err!("return_type() should not be called")
+ }
+
+ fn return_field_from_args(&self, args: datafusion_expr::ReturnFieldArgs) -> Result<FieldRef> {
+ normalize_field_for_geoparquet(&args.arg_fields[0], self.version, &self.crs_provider)
+ }
+
+ fn invoke_with_args(&self, args: datafusion_expr::ScalarFunctionArgs) -> Result<ColumnarValue> {
+ Ok(args.args[0].clone())
+ }
+}
+
+fn normalize_field_for_geoparquet(
+ field: &FieldRef,
+ version: GeoParquetVersion,
+ crs_provider: &CrsProviderOption,
+) -> Result<FieldRef> {
+ if field.metadata().is_empty() {
+ return Ok(field.clone());
+ }
+
+ let sedona_type = SedonaType::from_storage_field(field)?;
+ match sedona_type {
+ SedonaType::Arrow(DataType::Struct(children)) => {
+ let new_type = DataType::Struct(
+ children
+ .iter()
+ .map(|f| normalize_field_for_geoparquet(f, version, crs_provider))
+ .collect::<Result<_>>()?,
+ );
+ Ok(Arc::new(field.as_ref().clone().with_data_type(new_type)))
+ }
+ SedonaType::Arrow(DataType::List(child)) => {
+ let new_type = DataType::List(normalize_field_for_geoparquet(
+ &child,
+ version,
+ crs_provider,
+ )?);
+ Ok(Arc::new(field.as_ref().clone().with_data_type(new_type)))
+ }
Review Comment:
`normalize_field_for_geoparquet()` returns early when the *current* field has empty metadata, which prevents normalization of nested child fields (e.g., a Struct/List field with empty metadata but children that carry GeoArrow metadata). This breaks cases like Sedona's item-level CRS wrapper (a Struct with an `item` field that can be geo) and can cause GeoArrow metadata to leak into GeoParquet 1.0/1.1 outputs or skip CRS normalization/workarounds for 2.0/none. Consider removing the early return or recursing for Struct/List regardless of the parent field metadata (only skipping work once you’ve confirmed there is no nested metadata to normalize).
##########
rust/sedona-geoparquet/src/writer.rs:
##########
@@ -246,31 +251,27 @@ impl DataSink for GeoParquetSink {
data: SendableRecordBatchStream,
context: &Arc<TaskContext>,
) -> Result<u64> {
- if let Some(projection) = &self.projection {
- // If we have a projection, apply it here
- let schema = self.parquet_output_schema.clone();
- let projection = projection.clone();
-
- let data = Box::pin(RecordBatchStreamAdapter::new(
- schema.clone(),
- data.map(move |batch_result| {
- let schema = schema.clone();
-
- batch_result.and_then(|batch| {
- let mut columns = Vec::with_capacity(projection.len());
- for (expr, _) in &projection {
- let col = expr.evaluate(&batch)?;
- columns.push(col.into_array(batch.num_rows())?);
- }
- Ok(RecordBatch::try_new(schema.clone(), columns)?)
- })
- }),
- ));
+ // Apply the projection and write
+ let schema = self.parquet_output_schema.clone();
+ let projection = self.projection.clone();
+
+ let data = Box::pin(RecordBatchStreamAdapter::new(
+ schema.clone(),
+ data.map(move |batch_result| {
+ let schema = schema.clone();
+
+ batch_result.and_then(|batch| {
+ let mut columns = Vec::with_capacity(projection.len());
+ for (expr, _) in &projection {
+ let col = expr.evaluate(&batch)?;
+ columns.push(col.into_array(batch.num_rows())?);
+ }
+ Ok(RecordBatch::try_new(schema.clone(), columns)?)
+ })
+ }),
+ ));
Review Comment:
`GeoParquetSink::write_all()` always rebuilds a new `RecordBatch` by evaluating the full `projection`, even when the projection is effectively identity (e.g., V2.0/none when fields already have the desired metadata and no bbox columns were added). This adds avoidable per-batch CPU/memory overhead. Consider detecting an identity projection and delegating directly to `self.inner.write_all(data, ...)` in that case.
##########
rust/sedona/src/context.rs:
##########
@@ -188,7 +188,22 @@ impl SedonaContext {
state_builder = state_builder.with_query_planner(Arc::new(planner));
let mut state = state_builder.build();
+
+ // Register GeoParquet and try to initialize our statistics accumulator. It is OK if this fails
+ // because we already registered it, but we propagate other errors for safety.
state.register_file_format(Arc::new(GeoParquetFormatFactory::new()), true)?;
+ let init_result =
+ sedona_geoparquet::statistics_accumulator::SedonaGeoStatsAccumulatorFactory::try_init();
+ if let Err(init_err) = init_result {
+ if !matches!(init_err, DataFusionError::ParquetError(_))
+ || !init_err
+ .to_string()
+ .contains("GeoStatsAccumulatorFactory already set")
+ {
+ return Err(init_err);
+ }
+ }
Review Comment:
The GeoStats accumulator initialization failure check relies on matching a substring in `init_err.to_string()` ("GeoStatsAccumulatorFactory already set"). This is brittle across parquet/datafusion versions and can cause context construction to fail unexpectedly if the error wording changes. Prefer matching the concrete error variant/details (e.g., ParquetError kind) or exposing/using an API that reports "already initialized" without string parsing.
--
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] feat(rust/sedona-geoparquet): Add GeoParquet 2.0/Parquet-native geometry and geography support to Parquet writer [sedona-db]
Posted by "zhangfengcdt (via GitHub)" <gi...@apache.org>.
zhangfengcdt commented on PR #805:
URL: https://github.com/apache/sedona-db/pull/805#issuecomment-4400709903
> > Is there any reason we cannot just do the following?
>
> `.to_parquet()` is the idiom in Pandas, GeoPandas, DuckDB, and Ibis. Personally I like it because autocomplete works when working interactively (parameter documentation is highlighted as you type).
>
> Feel free to open an issue about adding a standalone Python package implementing a Spark compatibility layer like DuckDB's to see if there is interest!
Sure, I will create a ticket on that.
--
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] feat(rust/sedona-geoparquet): Add GeoParquet 2.0/Parquet-native geometry and geography support to Parquet writer [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #805:
URL: https://github.com/apache/sedona-db/pull/805
--
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] feat(rust/sedona-geoparquet): Add GeoParquet 2.0/Parquet-native geometry and geography support to Parquet writer [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #805:
URL: https://github.com/apache/sedona-db/pull/805#issuecomment-4399280785
> Is there any reason we cannot just do the following?
`.to_parquet()` is the idiom in Pandas, GeoPandas, DuckDB, and Ibis. Personally I like it because autocomplete works when working interactively (parameter documentation is highlighted as you type).
Feel free to open an issue about adding a standalone Python package implementing a Spark compatibility layer like DuckDB's to see if there is interest!
--
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] feat(rust/sedona-geoparquet): Add GeoParquet 2.0/Parquet-native geometry and geography support to Parquet writer [sedona-db]
Posted by "zhangfengcdt (via GitHub)" <gi...@apache.org>.
zhangfengcdt commented on PR #805:
URL: https://github.com/apache/sedona-db/pull/805#issuecomment-4398532319
I am looking at this code sample
```
sd.sql(
"SELECT ST_SetSRID(ST_GeogFromWKB(ST_AsBinary(geometry)), 4326) as geog FROM pts"
).to_parquet("geog.parquet", geoparquet_version="2.0")
```
Is there any reason we cannot just do the following?
df.write.option("geoparquet_version", "2.0").parquet("path/to/destination")
--
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] feat(rust/sedona-geoparquet): Add GeoParquet 2.0/Parquet-native geometry and geography support to parque writer [sedona-db]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #805:
URL: https://github.com/apache/sedona-db/pull/805#discussion_r3192472434
##########
rust/sedona/src/context.rs:
##########
@@ -188,7 +188,13 @@ impl SedonaContext {
state_builder = state_builder.with_query_planner(Arc::new(planner));
let mut state = state_builder.build();
+
+ // Register GeoParquet and try to initialize our statistics accumulator. It is OK if this fails
+ // (is likely because we already registered it).
state.register_file_format(Arc::new(GeoParquetFormatFactory::new()), true)?;
+ let _ =
+ sedona_geoparquet::statistics_accumulator::SedonaGeoStatsAccumulatorFactory::try_init();
Review Comment:
The result of SedonaGeoStatsAccumulatorFactory::try_init() is discarded unconditionally. This can hide real initialization failures (e.g., if the Parquet global factory cannot be installed) and leave GeoParquet stats silently disabled. Consider ignoring only the expected "already initialized" case and surfacing/logging other errors.
##########
rust/sedona-geoparquet/src/writer.rs:
##########
@@ -532,8 +574,183 @@ fn append_float_bbox(
Ok(())
}
+#[derive(Debug, PartialEq)]
+struct NormalizeForGeoParquet {
+ crs_provider: CrsProviderOption,
+ version: GeoParquetVersion,
+ signature: Signature,
+}
+
+impl NormalizeForGeoParquet {
+ fn new(crs_provider: CrsProviderOption, version: GeoParquetVersion) -> Self {
+ Self {
+ crs_provider,
+ version,
+ signature: Signature::any(1, Volatility::Stable),
+ }
+ }
+}
+
+impl std::hash::Hash for NormalizeForGeoParquet {
+ fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+ self.version.hash(state);
+ self.signature.hash(state);
+ }
+}
+
+impl Eq for NormalizeForGeoParquet {}
+
+impl ScalarUDFImpl for NormalizeForGeoParquet {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn name(&self) -> &str {
+ "normalize_for_geoparquet"
+ }
+
+ fn signature(&self) -> &Signature {
+ &self.signature
+ }
+
+ fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
+ sedona_internal_err!("return_type() should not be called")
+ }
+
+ fn return_field_from_args(&self, args: datafusion_expr::ReturnFieldArgs) -> Result<FieldRef> {
+ normalize_field_for_geoparquet(&args.arg_fields[0], self.version, &self.crs_provider)
+ }
+
+ fn invoke_with_args(&self, args: datafusion_expr::ScalarFunctionArgs) -> Result<ColumnarValue> {
+ Ok(args.args[0].clone())
+ }
+}
+
+fn normalize_field_for_geoparquet(
+ field: &FieldRef,
+ version: GeoParquetVersion,
+ crs_provider: &CrsProviderOption,
+) -> Result<FieldRef> {
+ if field.metadata().is_empty() {
+ return Ok(field.clone());
+ }
+
+ let sedona_type = SedonaType::from_storage_field(field)?;
+ match sedona_type {
+ SedonaType::Arrow(DataType::Struct(children)) => {
+ let new_type = DataType::Struct(
+ children
+ .iter()
+ .map(|f| normalize_field_for_geoparquet(f, version, crs_provider))
+ .collect::<Result<_>>()?,
+ );
+ Ok(Arc::new(field.as_ref().clone().with_data_type(new_type)))
+ }
+ SedonaType::Arrow(DataType::List(child)) => {
+ let new_type = DataType::List(normalize_field_for_geoparquet(
+ &child,
+ version,
+ crs_provider,
+ )?);
+ Ok(Arc::new(field.as_ref().clone().with_data_type(new_type)))
+ }
+ SedonaType::Arrow(_) => Ok(field.clone()),
+ SedonaType::Wkb(edges, crs) | SedonaType::WkbView(edges, crs) => match version {
+ // For GeoParquet 1.0 and 1.1, strip the metadata (we write Binary storage)
+ GeoParquetVersion::V1_0 | GeoParquetVersion::V1_1 => Ok(Arc::new(
+ field.as_ref().clone().with_metadata(HashMap::new()),
+ )),
+ // For GeoParquet 2.0 and None, ensure we have projjson CRS output
+ GeoParquetVersion::V2_0 | GeoParquetVersion::Omitted => {
+ let normalized_crs_value =
+ normalize_crs_for_geoparquet(field.name(), &crs, crs_provider)?;
+ let normalized_crs =
+ deserialize_crs_from_obj(&normalized_crs_value.unwrap_or(Value::Null))?;
+ Ok(serialize_edges_and_crs_with_parquet_bug(
+ field,
+ &normalized_crs,
+ edges,
+ ))
+ }
+ },
+ _ => exec_err!("Unsupported geometry output to Parquet: {sedona_type}"),
+ }
+}
+
+// Due to a bug in the parquet type conversion, we need to serialize invalid metadata for gegraphy
Review Comment:
Typo in comment: "gegraphy" should be "geography".
##########
rust/sedona-geoparquet/src/writer.rs:
##########
@@ -123,93 +152,69 @@ pub fn create_geoparquet_writer_physical_plan(
metadata.primary_column = field_names[output_geometry_primary].clone();
}
- // Apply all columns
- for i in output_geometry_column_indices {
- let f = conf.output_schema().field(i);
- let sedona_type = SedonaType::from_storage_field(f)?;
- let mut column_metadata = GeoParquetColumnMetadata::default();
-
- let (edge_type, crs) = match sedona_type {
- SedonaType::Wkb(edge_type, crs) | SedonaType::WkbView(edge_type, crs) => {
- (edge_type, crs)
+ // We skip writing the arrow metadata because we have to serialize invalid GeoArrow
+ // metadata in some cases to work around a bug in the parquet GeoArrow -> Parquet
+ // logical type conversion.
+ let mut parquet_options = options.inner.clone().with_skip_arrow_metadata(true);
+
+ // Create the column metadata and finalize the Parquet meatdata if we're omitting the GeoParquet metadata
Review Comment:
This comment says "if we're omitting the GeoParquet metadata", but the guarded block runs only when metadata.version is non-empty (i.e., when GeoParquet metadata is being written). Please update the comment to reflect the actual condition to avoid misleading future changes.
##########
python/sedonadb/tests/io/test_parquet.py:
##########
@@ -483,6 +483,104 @@ def test_write_geoparquet_geography(con, geoarrow_data):
assert table_roundtrip == table
+def test_write_geoparquet_2_0(con, geoarrow_data):
+ # Checks a read and write of geography (roundtrip, since nobody else can read/write)
+ path = (
+ geoarrow_data
+ / "natural-earth"
+ / "files"
+ / "natural-earth_countries_geo.parquet"
+ )
+ skip_if_not_exists(path)
+
+ table = con.read_parquet(path).to_arrow_table()
+
+ with tempfile.TemporaryDirectory() as td:
+ tmp_parquet = Path(td) / "tmp.parquet"
+ con.create_data_frame(table).to_parquet(tmp_parquet, geoparquet_version="2.0")
+
+ table_roundtrip = con.read_parquet(tmp_parquet).to_arrow_table()
+ assert table_roundtrip == table
+
+ # Check for metadata and logical type
+ file = parquet.ParquetFile(tmp_parquet)
+ file_kv_metadata = file.metadata.metadata
+ assert b"geo" in file_kv_metadata
+ geo_metadata = json.loads(file_kv_metadata[b"geo"])
+ assert geo_metadata["version"] == "2.0.0"
+
+ file.metadata.schema.column(2).logical_type.to_json() == '{"Type": "Geometry"}'
+
Review Comment:
This line performs a comparison but doesn't assert it, so the test won't fail if the logical type is wrong. Use an assertion (e.g., assert that the JSON matches) to actually validate the Parquet logical type.
##########
rust/sedona-geoparquet/src/writer.rs:
##########
@@ -123,93 +152,69 @@ pub fn create_geoparquet_writer_physical_plan(
metadata.primary_column = field_names[output_geometry_primary].clone();
}
- // Apply all columns
- for i in output_geometry_column_indices {
- let f = conf.output_schema().field(i);
- let sedona_type = SedonaType::from_storage_field(f)?;
- let mut column_metadata = GeoParquetColumnMetadata::default();
-
- let (edge_type, crs) = match sedona_type {
- SedonaType::Wkb(edge_type, crs) | SedonaType::WkbView(edge_type, crs) => {
- (edge_type, crs)
+ // We skip writing the arrow metadata because we have to serialize invalid GeoArrow
+ // metadata in some cases to work around a bug in the parquet GeoArrow -> Parquet
+ // logical type conversion.
+ let mut parquet_options = options.inner.clone().with_skip_arrow_metadata(true);
+
+ // Create the column metadata and finalize the Parquet meatdata if we're omitting the GeoParquet metadata
Review Comment:
Typo in comment: "meatdata" should be "metadata".
##########
python/sedonadb/tests/io/test_parquet.py:
##########
@@ -483,6 +483,104 @@ def test_write_geoparquet_geography(con, geoarrow_data):
assert table_roundtrip == table
+def test_write_geoparquet_2_0(con, geoarrow_data):
+ # Checks a read and write of geography (roundtrip, since nobody else can read/write)
+ path = (
+ geoarrow_data
+ / "natural-earth"
+ / "files"
+ / "natural-earth_countries_geo.parquet"
+ )
+ skip_if_not_exists(path)
+
+ table = con.read_parquet(path).to_arrow_table()
+
+ with tempfile.TemporaryDirectory() as td:
+ tmp_parquet = Path(td) / "tmp.parquet"
+ con.create_data_frame(table).to_parquet(tmp_parquet, geoparquet_version="2.0")
+
+ table_roundtrip = con.read_parquet(tmp_parquet).to_arrow_table()
+ assert table_roundtrip == table
+
+ # Check for metadata and logical type
+ file = parquet.ParquetFile(tmp_parquet)
+ file_kv_metadata = file.metadata.metadata
+ assert b"geo" in file_kv_metadata
+ geo_metadata = json.loads(file_kv_metadata[b"geo"])
+ assert geo_metadata["version"] == "2.0.0"
+
+ file.metadata.schema.column(2).logical_type.to_json() == '{"Type": "Geometry"}'
+
+
+def test_write_geoparquet_no_metadata(con, geoarrow_data):
+ # Checks a read and write of geography (roundtrip, since nobody else can read/write)
+ path = (
+ geoarrow_data
+ / "natural-earth"
+ / "files"
+ / "natural-earth_countries_geo.parquet"
+ )
+ skip_if_not_exists(path)
+
+ table = con.read_parquet(path).to_arrow_table()
+
+ with tempfile.TemporaryDirectory() as td:
+ tmp_parquet = Path(td) / "tmp.parquet"
+ con.create_data_frame(table).to_parquet(tmp_parquet, geoparquet_version="none")
+
+ table_roundtrip = con.read_parquet(tmp_parquet).to_arrow_table()
+ assert table_roundtrip == table
+
+ # Check for absent metadata and but correct logical type
+ file = parquet.ParquetFile(tmp_parquet)
+ file_kv_metadata = file.metadata.metadata
+ assert file_kv_metadata is None or b"geo" not in file_kv_metadata
+
+ file.metadata.schema.column(2).logical_type.to_json() == '{"Type": "Geometry"}'
+ geo_stats = file.metadata.row_group(0).column(2).geo_statistics
+ assert geo_stats is not None
+ assert geo_stats.geospatial_types == [3, 6]
+ assert geo_stats.xmin <= -180
+ assert geo_stats.xmax >= 180
+
+
+def test_write_geoparquet_geography_no_metadata(con, geoarrow_data):
+ # Checks a read and write of geography (roundtrip, since nobody else can read/write)
+ path = (
+ geoarrow_data
+ / "natural-earth"
+ / "files"
+ / "natural-earth_countries-geography_geo.parquet"
+ )
+ skip_if_not_exists(path)
+
+ table = con.read_parquet(path).to_arrow_table()
+
+ with tempfile.TemporaryDirectory() as td:
+ tmp_parquet = Path(td) / "tmp.parquet"
+ con.create_data_frame(table).to_parquet(tmp_parquet, geoparquet_version="none")
+
+ table_roundtrip = con.read_parquet(tmp_parquet).to_arrow_table()
+ assert table_roundtrip == table
+
+ # Check for absent metadata and but correct logical type
+ file = parquet.ParquetFile(tmp_parquet)
+ file_kv_metadata = file.metadata.metadata
+ assert file_kv_metadata is None or b"geo" not in file_kv_metadata
+
+ file.metadata.schema.column(2).logical_type.to_json() == '{"Type": "Geography"}'
Review Comment:
This line performs a comparison but doesn't assert it, so the test won't fail if the logical type is wrong. Use an assertion to validate the Parquet logical type.
##########
c/sedona-s2geography/src/geography.rs:
##########
@@ -95,10 +95,15 @@ unsafe impl<'a> Send for Geography<'a> {}
unsafe impl<'a> Sync for Geography<'a> {}
/// Factory for creating Geography objects from various formats
+#[derive(Debug)]
pub struct GeographyFactory {
ptr: *mut S2GeogFactory,
}
+// Safety: const methods are thread safe (although there aren't any const methods)
Review Comment:
The safety comment justifying Send/Sync says "const methods" are thread-safe, but GeographyFactory has no const/&self methods (all public APIs require &mut self). Please clarify the actual safety rationale (e.g., safe to move across threads / use from a single thread at a time) so the Send/Sync impl is correctly documented.
##########
c/sedona-s2geography/src/rect_bounder.rs:
##########
@@ -31,6 +31,15 @@ pub struct RectBounder {
ptr: *mut S2GeogRectBounder,
}
+impl std::fmt::Debug for RectBounder {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ f.debug_struct("RectBounder")
+ .field("ptr", &self.ptr)
+ .field("finish()", &self.finish())
Review Comment:
The Debug implementation calls finish(), which can be relatively expensive and may have side effects depending on the C++ implementation. Debug formatting should generally avoid executing potentially mutating or costly computations; consider only printing ptr/is_empty or cached state.
##########
python/sedonadb/tests/io/test_parquet.py:
##########
@@ -483,6 +483,104 @@ def test_write_geoparquet_geography(con, geoarrow_data):
assert table_roundtrip == table
+def test_write_geoparquet_2_0(con, geoarrow_data):
+ # Checks a read and write of geography (roundtrip, since nobody else can read/write)
+ path = (
+ geoarrow_data
+ / "natural-earth"
+ / "files"
+ / "natural-earth_countries_geo.parquet"
+ )
+ skip_if_not_exists(path)
+
+ table = con.read_parquet(path).to_arrow_table()
+
+ with tempfile.TemporaryDirectory() as td:
+ tmp_parquet = Path(td) / "tmp.parquet"
+ con.create_data_frame(table).to_parquet(tmp_parquet, geoparquet_version="2.0")
+
+ table_roundtrip = con.read_parquet(tmp_parquet).to_arrow_table()
+ assert table_roundtrip == table
+
+ # Check for metadata and logical type
+ file = parquet.ParquetFile(tmp_parquet)
+ file_kv_metadata = file.metadata.metadata
+ assert b"geo" in file_kv_metadata
+ geo_metadata = json.loads(file_kv_metadata[b"geo"])
+ assert geo_metadata["version"] == "2.0.0"
+
+ file.metadata.schema.column(2).logical_type.to_json() == '{"Type": "Geometry"}'
+
+
+def test_write_geoparquet_no_metadata(con, geoarrow_data):
+ # Checks a read and write of geography (roundtrip, since nobody else can read/write)
+ path = (
+ geoarrow_data
+ / "natural-earth"
+ / "files"
+ / "natural-earth_countries_geo.parquet"
+ )
+ skip_if_not_exists(path)
+
+ table = con.read_parquet(path).to_arrow_table()
+
+ with tempfile.TemporaryDirectory() as td:
+ tmp_parquet = Path(td) / "tmp.parquet"
+ con.create_data_frame(table).to_parquet(tmp_parquet, geoparquet_version="none")
+
+ table_roundtrip = con.read_parquet(tmp_parquet).to_arrow_table()
+ assert table_roundtrip == table
+
+ # Check for absent metadata and but correct logical type
+ file = parquet.ParquetFile(tmp_parquet)
+ file_kv_metadata = file.metadata.metadata
+ assert file_kv_metadata is None or b"geo" not in file_kv_metadata
+
+ file.metadata.schema.column(2).logical_type.to_json() == '{"Type": "Geometry"}'
+ geo_stats = file.metadata.row_group(0).column(2).geo_statistics
Review Comment:
This line performs a comparison but doesn't assert it, so the test won't fail if the logical type is wrong. Use an assertion to validate the Parquet logical type.
--
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] feat(rust/sedona-geoparquet): Add GeoParquet 2.0/Parquet-native geometry and geography support to parque writer [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #805:
URL: https://github.com/apache/sedona-db/pull/805#discussion_r3192449202
##########
python/sedonadb/tests/io/test_parquet.py:
##########
@@ -483,6 +483,104 @@ def test_write_geoparquet_geography(con, geoarrow_data):
assert table_roundtrip == table
+def test_write_geoparquet_2_0(con, geoarrow_data):
+ # Checks a read and write of geography (roundtrip, since nobody else can read/write)
+ path = (
+ geoarrow_data
+ / "natural-earth"
+ / "files"
+ / "natural-earth_countries_geo.parquet"
+ )
+ skip_if_not_exists(path)
+
+ table = con.read_parquet(path).to_arrow_table()
+
+ with tempfile.TemporaryDirectory() as td:
+ tmp_parquet = Path(td) / "tmp.parquet"
+ con.create_data_frame(table).to_parquet(tmp_parquet, geoparquet_version="2.0")
+
+ table_roundtrip = con.read_parquet(tmp_parquet).to_arrow_table()
+ assert table_roundtrip == table
+
+ # Check for metadata and logical type
+ file = parquet.ParquetFile(tmp_parquet)
+ file_kv_metadata = file.metadata.metadata
+ assert b"geo" in file_kv_metadata
+ geo_metadata = json.loads(file_kv_metadata[b"geo"])
+ assert geo_metadata["version"] == "2.0.0"
+
+ file.metadata.schema.column(2).logical_type.to_json() == '{"Type": "Geometry"}'
Review Comment:
GeoParquet 2.0!
##########
rust/sedona-geoparquet/src/statistics_accumulator.rs:
##########
@@ -0,0 +1,169 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::sync::Arc;
+
+use datafusion_common::Result;
+use parquet::{
+ basic::LogicalType,
+ geospatial::accumulator::{
+ init_geo_stats_accumulator_factory, GeoStatsAccumulator, GeoStatsAccumulatorFactory,
+ ParquetGeoStatsAccumulator, VoidGeoStatsAccumulator,
+ },
+ schema::types::ColumnDescPtr,
+};
+
+pub struct SedonaGeoStatsAccumulatorFactory;
+
+impl SedonaGeoStatsAccumulatorFactory {
+ pub fn try_init() -> Result<()> {
+ init_geo_stats_accumulator_factory(Arc::new(Self))?;
+ Ok(())
+ }
+}
+
+impl GeoStatsAccumulatorFactory for SedonaGeoStatsAccumulatorFactory {
+ fn new_accumulator(&self, descr: &ColumnDescPtr) -> Box<dyn GeoStatsAccumulator> {
+ if let Some(LogicalType::Geometry { .. }) = descr.logical_type_ref() {
+ return Box::new(ParquetGeoStatsAccumulator::default());
+ }
Review Comment:
To get Geography statistics written we have to do some plumbing to get s2geography's rectangle bounder called from deep within the depths of the Parquet crate. This is the mechanism I added in the PR that enabled Geometry statistics to be written...you can initialize an "accumlator factory" on startup that dishes out dynamic stats accumulators. It's a little unfortunate we need an s2geography dependency here but the fact that we can write stats at all is very cool.
##########
rust/sedona-geoparquet/src/writer.rs:
##########
@@ -77,40 +79,67 @@ pub fn create_geoparquet_writer_physical_plan(
}
// If there is no geometry, just use the inner implementation
- let mut output_geometry_column_indices = conf.output_schema().geometry_column_indices()?;
- if output_geometry_column_indices.is_empty() {
+ let input_geometry_column_indices = conf.output_schema().geometry_column_indices()?;
+ if input_geometry_column_indices.is_empty() {
Review Comment:
There is a lot of churn in this function, which prepares a plan before writing. Basically, we now *always* do a "projection" (that just modifies metadata) because we either have to strip the geoarrow.wkb type for GeoParquet 1.0 and 1.1 (Parquet with the geospatial feature would write that as Geometry and we want plain byte array with no logical type), canonicalize the CRS to write PROJJSON for sure in the logical type (GeoParquet 2.0/no GeoParquet), and/or create some invalid GeoArrow metadata to work around a parquet Rust bug (see above). Finally, sometimes we don't write the key/value metadata (no GeoParquet).
##########
python/sedonadb/tests/io/test_parquet.py:
##########
@@ -483,6 +483,104 @@ def test_write_geoparquet_geography(con, geoarrow_data):
assert table_roundtrip == table
+def test_write_geoparquet_2_0(con, geoarrow_data):
+ # Checks a read and write of geography (roundtrip, since nobody else can read/write)
+ path = (
+ geoarrow_data
+ / "natural-earth"
+ / "files"
+ / "natural-earth_countries_geo.parquet"
+ )
+ skip_if_not_exists(path)
+
+ table = con.read_parquet(path).to_arrow_table()
+
+ with tempfile.TemporaryDirectory() as td:
+ tmp_parquet = Path(td) / "tmp.parquet"
+ con.create_data_frame(table).to_parquet(tmp_parquet, geoparquet_version="2.0")
+
+ table_roundtrip = con.read_parquet(tmp_parquet).to_arrow_table()
+ assert table_roundtrip == table
+
+ # Check for metadata and logical type
+ file = parquet.ParquetFile(tmp_parquet)
+ file_kv_metadata = file.metadata.metadata
+ assert b"geo" in file_kv_metadata
+ geo_metadata = json.loads(file_kv_metadata[b"geo"])
+ assert geo_metadata["version"] == "2.0.0"
+
+ file.metadata.schema.column(2).logical_type.to_json() == '{"Type": "Geometry"}'
+
+
+def test_write_geoparquet_no_metadata(con, geoarrow_data):
+ # Checks a read and write of geography (roundtrip, since nobody else can read/write)
+ path = (
+ geoarrow_data
+ / "natural-earth"
+ / "files"
+ / "natural-earth_countries_geo.parquet"
+ )
+ skip_if_not_exists(path)
+
+ table = con.read_parquet(path).to_arrow_table()
+
+ with tempfile.TemporaryDirectory() as td:
+ tmp_parquet = Path(td) / "tmp.parquet"
+ con.create_data_frame(table).to_parquet(tmp_parquet, geoparquet_version="none")
+
+ table_roundtrip = con.read_parquet(tmp_parquet).to_arrow_table()
+ assert table_roundtrip == table
+
+ # Check for absent metadata and but correct logical type
+ file = parquet.ParquetFile(tmp_parquet)
+ file_kv_metadata = file.metadata.metadata
+ assert file_kv_metadata is None or b"geo" not in file_kv_metadata
+
+ file.metadata.schema.column(2).logical_type.to_json() == '{"Type": "Geometry"}'
+ geo_stats = file.metadata.row_group(0).column(2).geo_statistics
+ assert geo_stats is not None
+ assert geo_stats.geospatial_types == [3, 6]
+ assert geo_stats.xmin <= -180
+ assert geo_stats.xmax >= 180
+
+
+def test_write_geoparquet_geography_no_metadata(con, geoarrow_data):
+ # Checks a read and write of geography (roundtrip, since nobody else can read/write)
+ path = (
+ geoarrow_data
+ / "natural-earth"
+ / "files"
+ / "natural-earth_countries-geography_geo.parquet"
+ )
+ skip_if_not_exists(path)
+
+ table = con.read_parquet(path).to_arrow_table()
+
+ with tempfile.TemporaryDirectory() as td:
+ tmp_parquet = Path(td) / "tmp.parquet"
+ con.create_data_frame(table).to_parquet(tmp_parquet, geoparquet_version="none")
+
+ table_roundtrip = con.read_parquet(tmp_parquet).to_arrow_table()
+ assert table_roundtrip == table
+
+ # Check for absent metadata and but correct logical type
+ file = parquet.ParquetFile(tmp_parquet)
+ file_kv_metadata = file.metadata.metadata
+ assert file_kv_metadata is None or b"geo" not in file_kv_metadata
+
+ file.metadata.schema.column(2).logical_type.to_json() == '{"Type": "Geography"}'
+
+ # We should only have stats if s2geography is enabled
+ geo_stats = file.metadata.row_group(0).column(2).geo_statistics
+ if "s2geography" not in sedonadb.__features__:
+ assert geo_stats is None
+ else:
+ assert geo_stats is not None
Review Comment:
Geography statistics + Geography logical type!
##########
rust/sedona-geoparquet/src/writer.rs:
##########
@@ -532,8 +574,183 @@ fn append_float_bbox(
Ok(())
}
+#[derive(Debug, PartialEq)]
+struct NormalizeForGeoParquet {
+ crs_provider: CrsProviderOption,
+ version: GeoParquetVersion,
+ signature: Signature,
+}
+
+impl NormalizeForGeoParquet {
+ fn new(crs_provider: CrsProviderOption, version: GeoParquetVersion) -> Self {
+ Self {
+ crs_provider,
+ version,
+ signature: Signature::any(1, Volatility::Stable),
+ }
+ }
+}
+
+impl std::hash::Hash for NormalizeForGeoParquet {
+ fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+ self.version.hash(state);
+ self.signature.hash(state);
+ }
+}
+
+impl Eq for NormalizeForGeoParquet {}
+
+impl ScalarUDFImpl for NormalizeForGeoParquet {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn name(&self) -> &str {
+ "normalize_for_geoparquet"
+ }
+
+ fn signature(&self) -> &Signature {
+ &self.signature
+ }
+
+ fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
+ sedona_internal_err!("return_type() should not be called")
+ }
+
+ fn return_field_from_args(&self, args: datafusion_expr::ReturnFieldArgs) -> Result<FieldRef> {
+ normalize_field_for_geoparquet(&args.arg_fields[0], self.version, &self.crs_provider)
+ }
+
+ fn invoke_with_args(&self, args: datafusion_expr::ScalarFunctionArgs) -> Result<ColumnarValue> {
+ Ok(args.args[0].clone())
+ }
+}
+
+fn normalize_field_for_geoparquet(
+ field: &FieldRef,
+ version: GeoParquetVersion,
+ crs_provider: &CrsProviderOption,
+) -> Result<FieldRef> {
+ if field.metadata().is_empty() {
+ return Ok(field.clone());
+ }
+
+ let sedona_type = SedonaType::from_storage_field(field)?;
+ match sedona_type {
+ SedonaType::Arrow(DataType::Struct(children)) => {
+ let new_type = DataType::Struct(
+ children
+ .iter()
+ .map(|f| normalize_field_for_geoparquet(f, version, crs_provider))
+ .collect::<Result<_>>()?,
+ );
+ Ok(Arc::new(field.as_ref().clone().with_data_type(new_type)))
+ }
+ SedonaType::Arrow(DataType::List(child)) => {
+ let new_type = DataType::List(normalize_field_for_geoparquet(
+ &child,
+ version,
+ crs_provider,
+ )?);
+ Ok(Arc::new(field.as_ref().clone().with_data_type(new_type)))
+ }
+ SedonaType::Arrow(_) => Ok(field.clone()),
+ SedonaType::Wkb(edges, crs) | SedonaType::WkbView(edges, crs) => match version {
+ // For GeoParquet 1.0 and 1.1, strip the metadata (we write Binary storage)
+ GeoParquetVersion::V1_0 | GeoParquetVersion::V1_1 => Ok(Arc::new(
+ field.as_ref().clone().with_metadata(HashMap::new()),
+ )),
+ // For GeoParquet 2.0 and None, ensure we have projjson CRS output
+ GeoParquetVersion::V2_0 | GeoParquetVersion::Omitted => {
+ let normalized_crs_value =
+ normalize_crs_for_geoparquet(field.name(), &crs, crs_provider)?;
+ let normalized_crs =
+ deserialize_crs_from_obj(&normalized_crs_value.unwrap_or(Value::Null))?;
+ Ok(serialize_edges_and_crs_with_parquet_bug(
+ field,
+ &normalized_crs,
+ edges,
+ ))
+ }
+ },
+ _ => exec_err!("Unsupported geometry output to Parquet: {sedona_type}"),
+ }
+}
+
+// Due to a bug in the parquet type conversion, we need to serialize invalid metadata for gegraphy
+// fields. The conversion logic expects "algorithm" but the valid GeoArrow metadata we serialize
+// by default is "edges".
+// https://github.com/apache/arrow-rs/blob/f725bc9b955f23772a6a6d8a38c99a8b3f359116/parquet-geospatial/src/types.rs#L64-L66
+fn serialize_edges_and_crs_with_parquet_bug(
+ original_field: &FieldRef,
+ crs: &Crs,
+ edges: Edges,
+) -> FieldRef {
+ let crs_component = crs
+ .as_ref()
+ .map(|crs| format!(r#""crs":{}"#, crs.to_json()));
+
+ let edges_component = match edges {
+ Edges::Planar => None,
+ // This is where we apply the workaround relative to our usual
+ // serialize_edges_and_crs().
+ Edges::Spherical => Some(r#""algorithm":"spherical""#),
+ };
Review Comment:
I filed https://github.com/apache/arrow-rs/issues/9929 for this one
--
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