github-actions[bot] commented on code in PR #69032:
URL: https://github.com/apache/doris/pull/69032#discussion_r4367818659
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/BindExpressionTest.java:
##########
@@ -295,6 +305,67 @@ void testRowLevelMergeOutputIsCoercedByPosition() {
Assertions.assertTrue(exception.getMessage().contains("not aligned"),
exception.getMessage());
}
+ @Test
+ void testDeleteOnlyMergeDoesNotValidateNestedSpatialDataWrites() {
+ for (DataType spatial : ImmutableList.of(new GeometryType("OGC:CRS84"),
+ new GeographyType("OGC:CRS84", "spherical"))) {
+ ArrayType array = ArrayType.of(spatial);
+ LogicalOneRowRelation input = new LogicalOneRowRelation(new
RelationId(1), ImmutableList.of(
+ new Alias(new NullLiteral(array), "shapes")));
+ LogicalProject<?> source = new LogicalProject<>(ImmutableList.of(
+ new Alias(new TinyIntLiteral((byte) 1),
MergeOperation.OPERATION_COLUMN),
+ new Alias(new StringLiteral("row"),
Column.ICEBERG_ROWID_COL),
+ new Alias(input.getOutput().get(0), "shapes")), input);
+ List<Column> columns = ImmutableList.of(new Column("shapes",
array.toCatalogDataType()));
+ for (boolean writesDataFiles : new boolean[] {false, true}) {
+ LogicalExternalRowLevelMergeSink<?> sink = new
LogicalExternalRowLevelMergeSink<>(
+ Mockito.mock(ExternalDatabase.class),
Mockito.mock(ExternalTable.class),
+ "schema-1", columns, ImmutableList.of(),
writesDataFiles, true,
+ Optional.empty(), Optional.empty(), source);
+ if (writesDataFiles) {
+ AnalysisException exception =
Assertions.assertThrows(AnalysisException.class,
Review Comment:
[P2] Update the MERGE assertion for supported nested spatial writes
The `writesDataFiles=true` case uses exactly matching source/target
ARRAY<GEOMETRY/GEOGRAPHY> metadata, which this head now supports. Binding
passes the Variant-only check, performs same-type output coercion, and
`validateMergeSpatialTypes` recursively accepts the matching spatial leaf; the
former `Nested spatial columns` production diagnostic is gone. Consequently
this new test expects an AnalysisException that the binder no longer raises.
Assert successful binding for both matching cases, and use a kind/CRS/algorithm
mismatch for a negative data-write case while retaining the delete-only
control. This is separate from the earlier production delete-only MERGE finding.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/MultiDistinctCount.java:
##########
@@ -64,6 +64,18 @@ protected MultiDistinctCount(AggregateFunctionParams
functionParams) {
super(functionParams);
}
+ @Override
+ public void checkLegalityBeforeTypeCoercion() {
+ if (getArgumentType(0).isSpatialType()) {
Review Comment:
[P2] Reject nested spatial arguments to MULTI_DISTINCT_COUNT
`isSpatialType()` misses ARRAY<GEOMETRY/GEOGRAPHY>. For the regression's two
equivalent POINT(1 2) rows with opposite WKB byte orders, `SELECT
multi_distinct_count(array(geom)) FROM geometry_values WHERE id IN (101,102)`
passes this guard. This explicit aggregate has `distinct=false` and no grouping
key, so the recursive COUNT DISTINCT/group-key checks do not run. BE's
implemented TYPE_ARRAY uniq specialization hashes through ColumnArray ->
ColumnSpatial's raw WKB, counting the encodings separately rather than
rejecting unsupported spatial DISTINCT. This is distinct from the previous
scalar missing-factory and later MAP/list DISTINCT-lowering findings: this
ARRAY specialization executes successfully. Use a recursive spatial-type check
here and after rewrite, and cover nested arrays for both spatial kinds.
##########
be/src/core/data_type/data_type_spatial.cpp:
##########
@@ -0,0 +1,220 @@
+// 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.
+
+#include "core/data_type/data_type_spatial.h"
+
+#include <algorithm>
+#include <cctype>
+#include <cstring>
+#include <string_view>
+
+#include "agent/be_exec_version_manager.h"
+#include "common/exception.h"
+#include "core/assert_cast.h"
+#include "core/column/column_const.h"
+#include "core/data_type/data_type.h"
+#include "core/data_type_serde/data_type_varbinary_serde.h"
+#include "core/field.h"
+#include "core/string_view.h"
+#include "exprs/function/geo/wkb_parse.h"
+#include "gen_cpp/data.pb.h"
+
+namespace doris {
+
+namespace {
+
+std::string normalize_spatial_crs(std::string crs) {
+ // Imported ogc:crs84 has default semantics; leave custom CRS/property
references unchanged.
+ constexpr std::string_view default_crs = "OGC:CRS84";
+ if (std::ranges::equal(crs, default_crs, [](unsigned char left, unsigned
char right) {
+ return std::tolower(left) == std::tolower(right);
+ })) {
+ return std::string(default_crs);
+ }
+ return crs;
+}
+
+std::string normalize_spatial_algorithm(PrimitiveType type, std::string
algorithm) {
+ if (type == TYPE_GEOGRAPHY && algorithm.empty()) {
+ return "spherical";
+ }
+ std::ranges::transform(algorithm, algorithm.begin(),
+ [](unsigned char c) { return
static_cast<char>(std::tolower(c)); });
+ return algorithm;
+}
+
+class SpatialParquetSerDe final : public
DataTypeVarbinarySerDeBase<ColumnSpatial> {
+public:
+ SpatialParquetSerDe(int nesting_level, bool geographic)
+ : DataTypeVarbinarySerDeBase<ColumnSpatial>(nesting_level),
_geographic(geographic) {}
+
+ Status read_column_from_parquet(IColumn& column, ParquetDecodeSource&
source,
+ const ParquetDecodeContext& context,
size_t num_values,
+ ParquetMaterializationState& state) const
override {
+ const auto start = column.size();
+
RETURN_IF_ERROR(DataTypeVarbinarySerDeBase<ColumnSpatial>::read_column_from_parquet(
Review Comment:
[P2] Add spatial columns to native nullable-selection batching
This new leaf SerDe is reached through FileScannerV2's nullable-selection
fallback: `visit_nullable_expandable_column` in native/column_chunk_reader.cpp
supports ColumnVarbinary but omits ColumnSpatial, so
`supports_fused_nullable_selection` returns false. For a filtered
materialization range alternating Point/NULL, `decode_selected_values<true>`
enters the decoder and this SerDe separately for every retained CONTENT run.
The binary-carrier path instead batches compact non-NULL values and expands
NULLs within each materialized page fragment. Large filtered ranges with many
one-value runs therefore lose native batching. Add ownership-safe in-place
StringView expansion and register ColumnSpatial in this dispatcher; merely
adding the visitor entry is insufficient because ColumnSpatial lacks the
mutable expansion interface. Cover filtered plain/dictionary inputs with
alternating NULLs and verify that the fused path avoids
HybridSelectionNullFallbackBatches.
--
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]
