Copilot commented on code in PR #616:
URL: https://github.com/apache/sedona-db/pull/616#discussion_r2806319346
##########
rust/sedona-functions/src/st_geomfromwkb.rs:
##########
@@ -72,51 +70,13 @@ pub fn st_geogfromwkb_udf() -> SedonaScalarUDF {
out_type: WKB_VIEW_GEOGRAPHY,
})],
Volatility::Immutable,
- Some(doc("ST_GeogFromWKB", "Geography")),
- )
-}
-
-fn doc(name: &str, out_type_name: &str) -> Documentation {
- Documentation::builder(
- DOC_SECTION_OTHER,
- format!("Construct a {out_type_name} from WKB"),
- format!("{name} (Wkb: Binary)"),
- )
- .with_argument(
- "WKB",
- format!(
- "binary: Well-known binary representation of the {}",
- out_type_name.to_lowercase()
- ),
+ None,
)
- .with_sql_example(format!("SELECT {name}([01 02 00 00 00 02 00 00 00 00 00
00 00 84 D6 00 C0 00 00 00 00 80 B5 D6 BF 00 00 00 60 E1 EF F7 BF 00 00 00 80
07 5D E5 BF])"))
- .with_related_udf("ST_AsText")
- .build()
}
-/// Documentation for `ST_GeomFromWKBUnchecked()`.
+/// for `ST_GeomFromWKBUnchecked()`.
///
/// Parameterized for reuse if `ST_GeogFromWKBUnchecked()` is implemented in
the future.
-fn doc_unchecked(name: &str, out_type_name: &str) -> Documentation {
- Documentation::builder(
- DOC_SECTION_OTHER,
- format!(
- "Construct a {out_type_name} from WKB without validation. Invalid
WKB input may result in undefined behavior."
- ),
- format!("{name} (Wkb: Binary)"),
- )
- .with_argument(
- "WKB",
- format!(
- "binary: Well-known binary representation of the {}",
- out_type_name.to_lowercase()
- ),
- )
- .with_sql_example(format!("SELECT {name}([01 02 00 00 00 02 00 00 00 00 00
00 00 84 D6 00 C0 00 00 00 00 80 B5 D6 BF 00 00 00 60 E1 EF F7 BF 00 00 00 80
07 5D E5 BF])"))
- .with_related_udf("ST_AsText")
- .build()
-}
-
#[derive(Debug)]
Review Comment:
This rustdoc comment fragment (`/// for ST_GeomFromWKBUnchecked()`) no
longer describes what follows and reads like a leftover from the removed
Documentation helpers. Consider rewriting it to describe `STGeomFromWKB` (e.g.,
that it is the shared kernel for the WKB constructors) to avoid confusing
generated docs.
##########
docs/reference/sql/st_geomfromwkb.qmd:
##########
@@ -17,19 +17,43 @@
# under the License.
title: ST_GeomFromWKB
-description: Construct a Geometry from WKB.
+description: Construct a Geometry from Well-Known Binary (WKB).
kernels:
- returns: geometry
args:
- name: wkb
type: binary
+ - returns: geometry
+ args:
+ - name: wkb
+ type: binary
+ - name: srid
+ type: integer
Review Comment:
The second kernel’s `srid` argument is declared as `type: integer`, but the
page text/examples indicate it can accept an SRID *or* CRS string (e.g.,
'OGC:CRS27'). Consider using a CRS-capable type label here (e.g., `crs` or
`string`) and/or documenting both accepted forms to keep the rendered
Usage/Arguments accurate.
##########
docs/reference/sql/st_knn.qmd:
##########
@@ -21,14 +21,24 @@ description: Return true if geomA finds k nearest neighbors
from geomB.
kernels:
- returns: boolean
args:
- - geometry
+ - name: geomA
+ type: geometry
+ description: The geometry around which to search.
+ - name: geomb
+ type: geometry
+ description: Column containing candidate geometries.
- geometry
- name: k
type: integer
+ description: The number of nearest neighbours to return.
- name: use_spheroid
- type: boolean
+ type: true to use spherical distance, false for Euclidean
Review Comment:
The YAML frontmatter kernel signature looks inconsistent with the intended
ST_KNN call signature: there is an extra shorthand `- geometry` arg (which will
render as an unexpected third geometry parameter), `geomb` should likely be
`geomB`, and `use_spheroid`'s `type` field currently contains prose instead of
a type (e.g., `boolean`). Please update the `args` list so it matches
`ST_KNN(geomA, geomB, k, use_spheroid)` and move the prose into `description`
fields.
##########
docs/reference/sql/st_point.qmd:
##########
@@ -27,8 +27,18 @@ kernels:
type: double
---
+## Description
+
+Constructs a Point geometry from X and Y coordinates. An optional SRID or CRS
can be provided as a third argument.
+
Review Comment:
The Description/Examples mention an optional third SRID/CRS argument, but
the frontmatter `kernels` for this page only documents the 2-arg form (x, y).
Please add a kernel variant for the 3-argument overload so generated
Usage/Arguments match what’s described here.
##########
docs/reference/sql/st_geogpoint.qmd:
##########
@@ -27,6 +27,11 @@ kernels:
type: double
---
+## Description
+
+Creates a geography Point from longitude and latitude coordinates. An optional
+SRID or CRS can be provided as a third argument.
+
Review Comment:
This Description states an optional third SRID/CRS argument is supported,
but the frontmatter `kernels` only defines the 2-arg signature. Please add a
kernel variant including the SRID/CRS argument so the rendered Usage/Arguments
reflect the documented behavior.
--
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]
