Copilot commented on code in PR #3133:
URL: https://github.com/apache/sedona/pull/3133#discussion_r3619882026
##########
spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/io/netcdfmetadata/NetCdfMetadataPartitionReader.scala:
##########
@@ -569,11 +571,79 @@ class NetCdfMetadataPartitionReader(
.orElse(Option(globalAttrs.findAttributeString("crs_wkt", null)))
.orElse(Option(globalAttrs.findAttributeString("spatial_ref", null)))
.orNull
- val srid =
- if (!needSrid) None
- else if (wkt != null) lookupSrid(wkt)
- else gmVar.flatMap(latitudeLongitudeSrid)
- (wkt, srid)
+ if (wkt != null) {
+ (wkt, if (needSrid) lookupSrid(wkt) else None)
+ } else {
+ gmVar.flatMap(v => translateGridMapping(v, gridVar)) match {
+ case Some(proj) =>
+ (Try(proj.toWkt2).getOrElse(null), if (needSrid) projSrid(proj) else
None)
+ case None => (null, None)
+ }
Review Comment:
If proj4sedona successfully translates the grid mapping but `proj.toWkt2`
throws, this currently returns `(crs = null, srid = Some(...))`. That can yield
an SRID without a CRS string, which is inconsistent with the `srid` column
being derived from the resolved CRS and makes downstream behavior harder to
reason about. Prefer failing closed: only return a derived SRID when you can
also return the derived WKT (or else return neither).
##########
spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/io/netcdfmetadata/NetCdfMetadataPartitionReader.scala:
##########
@@ -569,11 +571,79 @@ class NetCdfMetadataPartitionReader(
.orElse(Option(globalAttrs.findAttributeString("crs_wkt", null)))
.orElse(Option(globalAttrs.findAttributeString("spatial_ref", null)))
.orNull
- val srid =
- if (!needSrid) None
- else if (wkt != null) lookupSrid(wkt)
- else gmVar.flatMap(latitudeLongitudeSrid)
- (wkt, srid)
+ if (wkt != null) {
+ (wkt, if (needSrid) lookupSrid(wkt) else None)
+ } else {
+ gmVar.flatMap(v => translateGridMapping(v, gridVar)) match {
+ case Some(proj) =>
+ (Try(proj.toWkt2).getOrElse(null), if (needSrid) projSrid(proj) else
None)
+ case None => (null, None)
+ }
+ }
+ }
+
+ /**
+ * CRS defined through CF grid mapping parameters, translated to a
projection via proj4sedona
+ * (pure Java — keeps this data source free of the optional LGPL GeoTools
runtime). CF requires
+ * `false_easting`/`false_northing` to be in the units of the projection
coordinates, so those
+ * units ride along. A `latitude_longitude` mapping is translated only when
its attributes
+ * positively identify the Earth figure (a resolvable datum or ellipsoid
name, explicit figure
+ * parameters, or `towgs84`) — the WGS 84 assumption GDAL and pyproj apply
to a bare geographic
+ * mapping must not be reported as if the file declared it. Unsupported or
malformed mappings
+ * translate to nothing rather than failing the row.
+ */
+ private def translateGridMapping(gmVar: Variable, gridVar:
Option[Variable]): Option[Proj] = {
+ Option(attrString(gmVar, "grid_mapping_name")).flatMap { name =>
+ Try {
+ val attrs = cfAttributeMap(gmVar)
+ if (name.trim.equalsIgnoreCase("latitude_longitude") &&
+ !CfGridMapping.identifiesEarthShape(attrs)) {
+ null
+ } else {
+ val (xUnits, yUnits) =
+ gridVar.map(projectionCoordinateUnits).getOrElse((null, null))
+ CfGridMapping.toProj(attrs, xUnits, yUnits)
+ }
+ }.toOption.flatMap(Option(_))
+ }
+ }
+
+ /**
+ * The grid mapping variable's attributes as the value shapes proj4sedona
coerces: strings stay
+ * strings, single numerics become `Number`, and multi-valued numerics (e.g.
the two standard
+ * parallels) become `double[]`. Unsigned values are widened to their
positive representation;
+ * non-numeric array attributes are skipped rather than misread.
+ */
+ private def cfAttributeMap(gmVar: Variable): java.util.Map[String, Object] =
{
+ val map = new java.util.HashMap[String, Object]()
+ gmVar.attributes().asScala.foreach { attr =>
+ val value: Object =
+ if (attr.isString) attr.getStringValue
+ else if (attr.getLength == 1) numericAttrValue(attr, 0)
+ else {
+ val values = new Array[Double](attr.getLength)
+ val numeric = (0 until attr.getLength).forall { i =>
+ val n = numericAttrValue(attr, i)
+ if (n != null) values(i) = n.doubleValue()
+ n != null
+ }
+ if (numeric) values else null
+ }
+ if (value != null) map.put(attr.getShortName, value)
+ }
+ map
+ }
+
+ /**
+ * The `units` attributes of the grid's x and y projection coordinate
variables. CF declares
+ * `false_easting` in the x unit and `false_northing` in the y unit;
proj4sedona accepts
+ * equivalent spellings and rejects incompatible axes rather than misscaling
either offset.
+ */
+ private def projectionCoordinateUnits(gridVar: Variable): (String, String) =
{
+ val (latDim, lonDim) = trailingDims(gridVar)
+ def unitsOf(dim: Dimension): String =
+ findCoordinateVariable(gridVar, dim).map(v => attrString(v,
"units")).orNull
+ (unitsOf(lonDim), unitsOf(latDim))
}
Review Comment:
`trailingDims` returns the grid's trailing dimensions as (y, x), but
`projectionCoordinateUnits` names them `(latDim, lonDim)` and then returns
`(unitsOf(lonDim), unitsOf(latDim))`. The logic is correct, but the names are
misleading and make it easy to accidentally swap axes in future edits. Consider
renaming to `(yDim, xDim)` to match the (y, x) convention used elsewhere in
this reader.
##########
pom.xml:
##########
@@ -96,7 +96,7 @@
<scala-collection-compat.version>2.5.0</scala-collection-compat.version>
<geoglib.version>1.52</geoglib.version>
<caffeine.version>2.9.2</caffeine.version>
- <proj4sedona.version>0.1.1</proj4sedona.version>
+ <proj4sedona.version>0.1.2</proj4sedona.version>
Review Comment:
Bumping `proj4sedona.version` to `0.1.2` will make Maven resolution depend
on that artifact being available in the configured repositories (this build
points at Maven Central/OSGeo/Unidata/Spark Packages). The PR description notes
`0.1.2` still needs to be published to Maven Central; until that happens, CI
and downstream builds will fail to resolve this dependency. Please ensure the
release is published (or add an appropriate temporary repository/staging
mechanism) before merging.
--
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]
