Gabriel39 commented on code in PR #67907:
URL: https://github.com/apache/doris/pull/67907#discussion_r4001678260
##########
be/src/format/transformer/vparquet_transformer.cpp:
##########
@@ -279,6 +387,12 @@ Status VParquetTransformer::write(const Block& block) {
return Status::OK();
}
+ if (_iceberg_schema != nullptr) {
+ ColumnNumbers column_numbers(block.columns());
+ std::iota(column_numbers.begin(), column_numbers.end(), 0);
+ RETURN_IF_ERROR(validate_spatial_wkb_inputs(block, column_numbers));
Review Comment:
[P1] Do not require S2 longitude/latitude semantics for raw GEOMETRY writes
This call validates all spatial payloads through `decode_geo_shape()` ->
`GeoShape::from_wkb_bytes()` -> `WkbParse::readPoint()` ->
`GeoPoint::from_coord()` -> `to_s2point()`. The last step requires `abs(x) <=
180 && abs(y) <= 90`, regardless of the column's CRS.
For example, a valid `GEOMETRY(EPSG:3857)` value containing `POINT (1000
2000)` cannot be copied with `INSERT INTO spatial_target SELECT geom FROM
spatial_source`, even when both schemas have exactly the same CRS: FE
validation succeeds, but this writer rejects the payload as invalid WKB.
`ST_AsText` and `ST_X` encounter the same restriction through the shared
validator. The existing non-default CRS write test uses `POINT (1 2)`, which
does not expose this.
Please separate WKB structural validation/raw preservation from S2
computation support. Copying a projected geometry must not require its
coordinates to be valid longitude/latitude. Iceberg defines GEOMETRY as
CRS-parameterized with planar edges:
https://iceberg.apache.org/spec/#primitive-types
##########
be/src/exprs/function/geo/functions_geo.cpp:
##########
@@ -32,15 +33,128 @@
#include "core/column/column_nullable.h"
#include "core/data_type/data_type_nullable.h"
#include "core/data_type/data_type_number.h"
+#include "core/data_type/data_type_spatial.h"
#include "core/data_type/data_type_string.h"
#include "core/data_type/define_primitive_type.h"
#include "core/string_ref.h"
#include "exprs/function/geo/geo_common.h"
#include "exprs/function/geo/geo_types.h"
#include "exprs/function/simple_function_factory.h"
+#include "exprs/function/string_hex_util.h"
namespace doris {
+static bool is_spatial_type(const DataTypePtr& type) {
+ const auto primitive_type = remove_nullable(type)->get_primitive_type();
+ return primitive_type == TYPE_GEOMETRY || primitive_type == TYPE_GEOGRAPHY;
+}
+
+static Status validate_geography_semantics(const DataTypePtr& type, const
char* function_name) {
+ const auto& nested_type = remove_nullable(type);
+ if (!is_spatial_type(nested_type)) {
+ return Status::OK();
+ }
+
+ const auto* spatial_type = dynamic_cast<const
DataTypeSpatial*>(nested_type.get());
+ DCHECK(spatial_type != nullptr);
+ if (spatial_type != nullptr && spatial_type->get_primitive_type() ==
TYPE_GEOGRAPHY &&
+ spatial_type->crs() == "OGC:CRS84" && spatial_type->algorithm() ==
"spherical") {
+ return Status::OK();
+ }
+ return Status::NotSupported(
+ "Function {} requires GEOGRAPHY(OGC:CRS84, spherical) for spatial
inputs",
+ function_name);
+}
+
+static std::unique_ptr<GeoShape> decode_geo_shape(StringRef value, const
DataTypePtr& type,
+ GeoParseStatus* parse_status
= nullptr) {
+ if (!is_spatial_type(type)) {
+ return GeoShape::from_encoded(value.data, value.size);
+ }
+
+ GeoParseStatus status;
+ auto shape = GeoShape::from_wkb_bytes(value.data, value.size, status);
+ if (parse_status != nullptr) {
+ *parse_status = status;
+ }
+ return status == GEO_PARSE_OK ? std::move(shape) : nullptr;
+}
+
+static bool has_unsupported_spatial_wkb_metadata(StringRef value) {
+ if (value.size < 5) {
+ return false;
+ }
+
+ const auto byte_order = static_cast<uint8_t>(value.data[0]);
+ if (byte_order != 0 && byte_order != 1) {
+ return false;
+ }
+
+ const auto byte_at = [&value](size_t offset) {
+ return static_cast<uint8_t>(value.data[offset]);
+ };
+ const uint32_t type = byte_order == 1 ? static_cast<uint32_t>(byte_at(1)) |
+
(static_cast<uint32_t>(byte_at(2)) << 8) |
+
(static_cast<uint32_t>(byte_at(3)) << 16) |
+
(static_cast<uint32_t>(byte_at(4)) << 24)
+ : (static_cast<uint32_t>(byte_at(1))
<< 24) |
+
(static_cast<uint32_t>(byte_at(2)) << 16) |
+
(static_cast<uint32_t>(byte_at(3)) << 8) |
+
static_cast<uint32_t>(byte_at(4));
+
+ 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 ((type & ewkb_metadata_flags) != 0) {
+ return true;
+ }
+
+ return type >= 1000 && type < 4000;
+}
+
+static bool decode_wkb_hex(StringRef value, std::string* wkb) {
+ const char* data = value.data;
+ size_t size = value.size;
+ if (size >= 2 && ((data[0] == '0' && data[1] == 'x') || (data[0] == '\\'
&& data[1] == 'x'))) {
+ data += 2;
+ size -= 2;
+ }
+ if (size == 0 || (size & 1) != 0) {
+ return false;
+ }
+ wkb->resize(size / 2);
+ return string_hex::hex_decode(data, size, wkb->data()) == size / 2;
+}
+
+Status validate_spatial_wkb_inputs(const Block& block, const ColumnNumbers&
arguments) {
+ for (const auto argument : arguments) {
+ const auto& column = block.get_by_position(argument).column;
+ const auto& type = block.get_data_type(argument);
+ if (!is_spatial_type(type)) {
+ continue;
+ }
+ for (size_t row = 0; row < column->size(); ++row) {
+ if (column->is_null_at(row)) {
Review Comment:
[P1] Preserve the null map when validating spatial arguments
`GeoFunction` uses the default nullable implementation.
`PreparedFunctionImpl::default_implementation_for_nulls()` calls
`unnest_nullable()` before invoking `execute_impl()`, so this validator
receives the nested `ColumnSpatial`, whose `is_null_at()` always returns false.
A NULL row with the default empty payload is therefore parsed as WKB and
returns `InvalidArgument` before the framework can merge the original null map
into the result.
A batch containing both a valid geometry and NULL will fail for `SELECT
ST_AsText(geom) FROM spatial_source`. The same path is reachable through
`ST_AsText(ST_GeomFromWKB(wkb))` when a nonconstant input column contains both
valid and invalid WKB: the constructor produces a NULL with an empty payload
for the invalid row, and the outer function rejects it. An all-NULL test can
miss this because the framework may short-circuit it.
Please preserve the original null information at the validation boundary and
add a mixed NULL/non-NULL function test, while retaining explicit errors for
invalid non-NULL spatial values.
##########
be/src/exprs/function/geo/functions_geo.cpp:
##########
@@ -32,15 +33,128 @@
#include "core/column/column_nullable.h"
#include "core/data_type/data_type_nullable.h"
#include "core/data_type/data_type_number.h"
+#include "core/data_type/data_type_spatial.h"
#include "core/data_type/data_type_string.h"
#include "core/data_type/define_primitive_type.h"
#include "core/string_ref.h"
#include "exprs/function/geo/geo_common.h"
#include "exprs/function/geo/geo_types.h"
#include "exprs/function/simple_function_factory.h"
+#include "exprs/function/string_hex_util.h"
namespace doris {
+static bool is_spatial_type(const DataTypePtr& type) {
+ const auto primitive_type = remove_nullable(type)->get_primitive_type();
+ return primitive_type == TYPE_GEOMETRY || primitive_type == TYPE_GEOGRAPHY;
+}
+
+static Status validate_geography_semantics(const DataTypePtr& type, const
char* function_name) {
+ const auto& nested_type = remove_nullable(type);
+ if (!is_spatial_type(nested_type)) {
+ return Status::OK();
+ }
+
+ const auto* spatial_type = dynamic_cast<const
DataTypeSpatial*>(nested_type.get());
+ DCHECK(spatial_type != nullptr);
+ if (spatial_type != nullptr && spatial_type->get_primitive_type() ==
TYPE_GEOGRAPHY &&
+ spatial_type->crs() == "OGC:CRS84" && spatial_type->algorithm() ==
"spherical") {
+ return Status::OK();
+ }
+ return Status::NotSupported(
+ "Function {} requires GEOGRAPHY(OGC:CRS84, spherical) for spatial
inputs",
+ function_name);
+}
+
+static std::unique_ptr<GeoShape> decode_geo_shape(StringRef value, const
DataTypePtr& type,
+ GeoParseStatus* parse_status
= nullptr) {
+ if (!is_spatial_type(type)) {
+ return GeoShape::from_encoded(value.data, value.size);
+ }
+
+ GeoParseStatus status;
+ auto shape = GeoShape::from_wkb_bytes(value.data, value.size, status);
+ if (parse_status != nullptr) {
+ *parse_status = status;
+ }
+ return status == GEO_PARSE_OK ? std::move(shape) : nullptr;
+}
+
+static bool has_unsupported_spatial_wkb_metadata(StringRef value) {
+ if (value.size < 5) {
+ return false;
+ }
+
+ const auto byte_order = static_cast<uint8_t>(value.data[0]);
+ if (byte_order != 0 && byte_order != 1) {
+ return false;
+ }
+
+ const auto byte_at = [&value](size_t offset) {
+ return static_cast<uint8_t>(value.data[offset]);
+ };
+ const uint32_t type = byte_order == 1 ? static_cast<uint32_t>(byte_at(1)) |
+
(static_cast<uint32_t>(byte_at(2)) << 8) |
+
(static_cast<uint32_t>(byte_at(3)) << 16) |
+
(static_cast<uint32_t>(byte_at(4)) << 24)
+ : (static_cast<uint32_t>(byte_at(1))
<< 24) |
+
(static_cast<uint32_t>(byte_at(2)) << 16) |
+
(static_cast<uint32_t>(byte_at(3)) << 8) |
+
static_cast<uint32_t>(byte_at(4));
+
+ 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 ((type & ewkb_metadata_flags) != 0) {
+ return true;
+ }
+
+ return type >= 1000 && type < 4000;
+}
+
+static bool decode_wkb_hex(StringRef value, std::string* wkb) {
+ const char* data = value.data;
+ size_t size = value.size;
+ if (size >= 2 && ((data[0] == '0' && data[1] == 'x') || (data[0] == '\\'
&& data[1] == 'x'))) {
+ data += 2;
+ size -= 2;
+ }
+ if (size == 0 || (size & 1) != 0) {
+ return false;
+ }
+ wkb->resize(size / 2);
+ return string_hex::hex_decode(data, size, wkb->data()) == size / 2;
+}
+
+Status validate_spatial_wkb_inputs(const Block& block, const ColumnNumbers&
arguments) {
+ for (const auto argument : arguments) {
+ const auto& column = block.get_by_position(argument).column;
+ const auto& type = block.get_data_type(argument);
+ if (!is_spatial_type(type)) {
+ continue;
+ }
+ for (size_t row = 0; row < column->size(); ++row) {
+ if (column->is_null_at(row)) {
+ continue;
+ }
+ const auto value = column->get_data_at(row);
+ if (has_unsupported_spatial_wkb_metadata(value)) {
+ return Status::NotSupported(
+ "WKB dimensions or embedded SRID are not supported for
spatial inputs at "
+ "row {}",
+ row);
+ }
+ GeoParseStatus parse_status;
+ if (decode_geo_shape(value, type, &parse_status) == nullptr) {
Review Comment:
[P2] Reuse decoded shapes and validate constant arguments once
The pre-validation pass fully parses every WKB value and immediately
destroys the resulting `GeoShape`; implementations such as `StAsText` and
`StDistance` then decode the same payload again. This includes stream-buffer
copies, coordinate parsing, and S2 object construction, which is expensive for
large polygons.
It also defeats the constant-argument optimization in `StDistance`: for
`ST_Distance(geog_column, constant_geography)`, a `ColumnConst` still reports
the logical batch size, so this loop decodes the same constant WKB N times
before `StDistance` reaches its decode-once branch.
Please combine validation with decoding and reuse the result, handling
constant arguments once per execution rather than once per logical row. No
benchmark was run for this review, so this finding concerns the directly
observable redundant work rather than a measured slowdown.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/StGeomFromWKB.java:
##########
@@ -38,8 +39,8 @@ public class StGeomFromWKB extends ScalarFunction
implements UnaryExpression, ExplicitlyCastableSignature,
AlwaysNullable {
public static final List<FunctionSignature> SIGNATURES = ImmutableList.of(
-
FunctionSignature.ret(VarcharType.SYSTEM_DEFAULT).args(VarcharType.SYSTEM_DEFAULT),
-
FunctionSignature.ret(VarcharType.SYSTEM_DEFAULT).args(StringType.INSTANCE)
+ FunctionSignature.ret(new
GeometryType("OGC:CRS84")).args(VarcharType.SYSTEM_DEFAULT),
Review Comment:
[P1] Preserve compatibility of existing WKB constructor consumers
Changing the existing `ST_GeomFromWKB`/`ST_GeometryFromWKB` result from the
legacy VARCHAR spatial encoding to GEOMETRY breaks existing function
compositions outside Iceberg. The downstream `validate_geography_semantics()`
now explicitly rejects GEOMETRY for `ST_Distance`, `ST_Length`, `ST_Contains`,
and other measurement/relationship functions.
For example, this previously valid expression should return zero, but now
reaches the NotSupported error:
```sql
SELECT ST_Distance(
ST_GeomFromWKB('0101000000000000000000F03F0000000000000040'),
ST_GeomFromWKB('0101000000000000000000F03F0000000000000040'));
```
The new tests verify that GEOMETRY is rejected, but do not cover
compatibility of these pre-existing SQL functions. Please retain the existing
constructor contract via a separate typed constructor, or provide an explicit
compatibility path for existing consumers.
--
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]
