Gabriel39 commented on code in PR #67907:
URL: https://github.com/apache/doris/pull/67907#discussion_r4002293221
##########
be/src/exprs/function/geo/functions_geo.h:
##########
@@ -62,13 +64,15 @@ class GeoFunction : public IFunction {
String get_name() const override { return name; }
size_t get_number_of_arguments() const override { return Impl::NUM_ARGS; }
bool is_variadic() const override { return false; }
+ bool use_default_implementation_for_nulls() const override { return false;
}
Review Comment:
[P1] Keep NULL propagation for numeric GeoFunction implementations
Disabling the default nullable implementation for every `GeoFunction`
introduces incorrect results in existing numeric functions.
`StPoint::execute()` uses `ColumnView<TYPE_DOUBLE>::value_at()` without
checking `is_null_at()`. ColumnView can unwrap a nullable column, but
`value_at()` only reads the underlying payload; it does not propagate NULL. The
result null map starts at zero, and the framework no longer merges the input
null maps after execution.
For a nonconstant nullable DOUBLE column `x`, a NULL row with its default
zero payload makes `SELECT ST_AsText(ST_Point(x, 5)) FROM coordinates` produce
`POINT (0 5)` instead of NULL. `ST_Distance_Sphere`, `ST_Angle_Sphere`, and
`ST_Circle` use the same unchecked numeric access pattern.
Please restrict the custom nullable handling to the functions that need it,
or implement explicit null checks and propagation in every affected
implementation. Run the existing
`VGeoFunctionsTest.function_geo_st_point_test`, which already expects NULL for
`{Null(), 5}` and `{5, Null()}`, and cover nonconstant mixed-null batches and
constant/nullable combinations. A SQL NULL literal alone may be optimized away
by FE and miss this regression.
##########
be/src/exprs/function/geo/functions_geo.cpp:
##########
@@ -104,7 +226,7 @@ struct StAsText {
std::unique_ptr<GeoShape> shape;
for (int row = 0; row < size; ++row) {
auto shape_value = input->get_data_at(row);
- shape = GeoShape::from_encoded(shape_value.data, shape_value.size);
+ shape = decode_geo_shape(shape_value, input_type);
Review Comment:
[P1] Preserve projected coordinates in accessors as well as the write
validator
The new structural validator accepts projected GEOMETRY values, but the
accessor still calls `decode_geo_shape()` -> `GeoShape::from_wkb_bytes()` ->
`WkbParse::readPoint()` -> `GeoPoint::from_coord()` -> `to_s2point()`. The
final step still enforces longitude/latitude limits without consulting the CRS.
Consequently, a non-NULL `GEOMETRY(EPSG:3857)` containing `POINT (1000
2000)` can now pass validation and be copied to Parquet, but `ST_AsText(geom)`
returns NULL here. `ST_X`, `ST_Y`, `ST_GeometryType`, and `ST_AsBinary` share
the same decoding restriction. This changes the previous explicit validation
error into a silent NULL for valid projected data.
Please use coordinate-preserving WKB handling for accessors and exports; in
particular, exporting the existing raw WKB should not depend on constructing an
S2 shape. Add a Doris query test that verifies the non-NULL projected value's
text, coordinates, type, and binary export after reading it back. The current
Arrow writer round-trip test does not exercise these functions.
I independently compiled the original WKB validator source from this
revision and verified that it accepts `POINT (1000 2000)`. The downstream NULL
result follows from the call chain above; I have not run a complete Doris SQL
reproduction.
##########
be/src/exprs/function/geo/wkb_parse.cpp:
##########
@@ -175,12 +210,12 @@ std::unique_ptr<GeoShape>
WkbParse::readGeometry(WkbParseContext& ctx) {
uint32_t typeInt = ctx.dis.readUnsigned();
- // Check if geometry has SRID
- bool has_srid = (typeInt & WKB_SRID_FLAG) != 0;
-
- // Read SRID if present
- if (has_srid) {
- ctx.dis.readUnsigned(); // Read and store SRID if needed
+ constexpr uint32_t ewkb_z_flag = 0x80000000;
+ constexpr uint32_t ewkb_m_flag = 0x40000000;
+ constexpr uint32_t ewkb_srid_flag = 0x20000000;
+ constexpr uint32_t ewkb_metadata_flags = ewkb_z_flag | ewkb_m_flag |
ewkb_srid_flag;
+ if ((typeInt & ewkb_metadata_flags) != 0 || (typeInt >= 1000 &&
typeInt < 4000)) {
Review Comment:
[P1] Retain SRID-bearing EWKB support in the legacy constructors
Restoring the legacy constructors' VARCHAR result does not restore their
full input compatibility. `LegacyStGeoFromWkb` still calls
`GeoShape::from_wkb()` -> `WkbParse::parse_wkb()` -> this shared
`readGeometry()`, which now rejects the SRID flag unconditionally. Previously,
this path consumed the SRID and parsed the coordinates.
There is already a regression for this in
`regression-test/suites/query_p0/sql_functions/spatial_functions/test_gis_function.groovy:460`:
```sql
SELECT ST_AsText(ST_GeometryFromWKB(
'01010000208A11000068270210774C5D40B8DECA334C3B4240'));
```
Its checked-in expected result is `POINT (117.194767000297 36.46326301008)`.
The fixture's type is `0x20000001` with SRID 4490, so this branch rejects it
before reading the coordinates and the legacy constructor returns NULL.
Please separate the typed Iceberg WKB restrictions from legacy EWKB parsing,
and run the existing SQL regression. Add a focused BE test proving that both
legacy constructor aliases still accept this input while the new typed
constructor follows its explicitly documented SRID policy.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
