eldenmoon commented on code in PR #65561:
URL: https://github.com/apache/doris/pull/65561#discussion_r3576469907
##########
be/src/storage/segment/variant/variant_assembler.cpp:
##########
@@ -0,0 +1,711 @@
+// Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
存储层的修改如果本PR的功能没有依赖则都不在这个PR中携带（包括单测）
##########
be/src/storage/tablet/tablet_schema.cpp:
##########
@@ -20,9 +20,6 @@
#include <gen_cpp/Descriptors_types.h>
#include <gen_cpp/olap_file.pb.h>
#include <glog/logging.h>
-#include <google/protobuf/io/coded_stream.h>
Review Comment:
如果修改无关则去掉修改
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/EliminateOrderByKey.java:
##########
@@ -81,6 +82,10 @@ private static Plan eliminateWindow(LogicalWindow<Plan>
window) {
Alias alias = (Alias) expr;
WindowExpression windowExpression = (WindowExpression)
alias.child();
List<OrderExpression> orderExpressions =
windowExpression.getOrderKeys();
+ if (orderExpressions.stream()
Review Comment:
靠BE兜底，不用在这里修改
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/check/CheckCast.java:
##########
@@ -342,6 +346,52 @@ public List<ExpressionPatternMatcher<? extends
Expression>> buildRules() {
);
}
Review Comment:
不用关心V2逻辑
##########
fe/fe-common/src/main/java/org/apache/doris/common/Config.java:
##########
@@ -2013,7 +2013,7 @@ public class Config extends ConfigBase {
* Max data version of backends serialize block.
*/
@ConfField(mutable = false)
- public static int max_be_exec_version = 10;
Review Comment:
V1, V2无需兼容， FE不用考虑V2, 只用给对应的算子（group by等）给variant放行即可， 靠BE兜底不挂（可以报错）
##########
be/src/exprs/function/cast/variant_v2/cast_variant_v2.cpp:
##########
@@ -0,0 +1,196 @@
+// Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
把cast_variant_v2*文件都挪到variant_v2目录下
##########
be/src/exprs/variant_v2_execution.h:
##########
Review Comment:
这个文件没有必要， 直接用config::enable_variant_v2即可
##########
gensrc/thrift/PaloInternalService.thrift:
##########
@@ -507,6 +507,9 @@ struct TQueryOptions {
225: optional i64 runtime_filter_tree_publish_max_send_bytes = 268435456
226: optional bool enable_prune_nested_column = false;
+
Review Comment:
无需此session variable
##########
be/src/util/variant/variant_encoding.h:
##########
@@ -0,0 +1,85 @@
+// 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.
+
+#pragma once
+
+#include <cstddef>
+#include <cstdint>
+
+namespace doris {
Review Comment:
add code comment for related document or RFC
##########
be/src/exprs/function/function_variant_element_v2_string.cpp:
##########
@@ -0,0 +1,61 @@
+// 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 "common/exception.h"
+#include "core/assert_cast.h"
+#include "core/column/column_nullable.h"
+#include "core/column/column_string.h"
+#include "exprs/function/function_variant_element_v2_internal.h"
+#include "util/variant/variant_json.h"
+
+namespace doris::variant_element_v2_internal {
+
+Status encode_typed_string_variant_input(const ColumnVariantV2& source,
+ std::span<const uint8_t> outer_nulls,
+ ColumnVariantV2::MutablePtr* const
output) {
+ DORIS_CHECK(source.is_typed());
+ const auto& nullable = assert_cast<const
ColumnNullable&>(source.typed_column());
+ const auto& strings = assert_cast<const
ColumnString&>(nullable.get_nested_column());
+ DORIS_CHECK_EQ(strings.size(), source.size());
+
+ JsonToVariantOptions options = JsonToVariantOptions::current_config();
+ options.throw_on_invalid_json = false;
+ JsonToVariantEncoder encoder(options);
+ const StringRef null_json("null", 4);
+ for (size_t row = 0; row < source.size(); ++row) {
+ const bool is_null =
+ nullable.is_null_at(row) || (!outer_nulls.empty() &&
outer_nulls[row] != 0);
+ encoder.add_json(is_null ? null_json : strings.get_data_at(row));
+ }
+
+ VariantEncodedBlock block = encoder.finish_block();
+ auto encoded = ColumnVariantV2::create();
+ encoded->insert_encoded_block(block.view());
+ output->swap(encoded);
+ return Status::OK();
+}
+
+Status extract_typed_string_variant_element(const ColumnVariantV2& source,
Review Comment:
为什么是这个语义？
typed如果是string类型，不应该把string编码到out吗/或者直接放到output的typed，而不是extract？可能需要修正这个语义，
如果拿不准找我对下
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/ArrayAgg.java:
##########
@@ -55,6 +58,16 @@ private ArrayAgg(AggregateFunctionParams functionParams) {
super(functionParams);
}
+ @Override
+ public void checkLegalityBeforeTypeCoercion() {
Review Comment:
不用关心V2, 如果不支持靠BE兜底即可， 检查下别的类似逻辑
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -844,6 +844,7 @@ public String toString() {
@Deprecated
public static final String ENABLE_VARIANT_FLATTEN_NESTED =
"enable_variant_flatten_nested";
public static final String ENABLE_VARIANT_SCHEMA_AUTO_CAST =
"enable_variant_schema_auto_cast";
+ public static final String ENABLE_VARIANT_V2 = "enable_variant_v2";
Review Comment:
无需SessionVariable， 靠BE config
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/ExtractAndNormalizeWindowExpression.java:
##########
@@ -61,6 +62,13 @@ private Plan normalize(LogicalProject<Plan> project) {
ExpressionUtils.rewriteDownShortCircuit(project.getProjects(),
output -> {
if (output instanceof WindowExpression) {
WindowExpression windowExpression = (WindowExpression)
output;
+ boolean hasVariantPartitionKey =
windowExpression.getPartitionKeys().stream()
Review Comment:
靠BE兜底，不用在这里修改
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/FoldConstantRuleOnBE.java:
##########
@@ -323,6 +323,7 @@ private static Map<String, Expression> evalOnBE(Map<String,
Map<String, TExpr>>
tQueryOptions.setNewVersionUnixTimestamp(true);
tQueryOptions.setNewVersionPercentile(true);
tQueryOptions.setEnableStrictCast(SessionVariable.enableStrictCast());
+
tQueryOptions.setEnableVariantV2(context.getSessionVariable().enableVariantV2);
Review Comment:
不依赖session variable
##########
be/src/exprs/vcondition_expr.h:
##########
@@ -73,8 +73,8 @@ class VectorizedIfExpr : public VConditionExpr {
inline static const std::string IF_NAME = "if";
protected:
- Status _execute_impl_internal(Block& block, const ColumnNumbers&
arguments, uint32_t result,
- size_t input_rows_count) const;
+ Status _execute_impl_internal(RuntimeState* state, Block& block, const
ColumnNumbers& arguments,
Review Comment:
这些基础代码的逻辑评估是否能不修改
##########
be/src/exprs/short_circuit_util.h:
##########
@@ -321,8 +323,8 @@ struct NonScalarFillWithSelector {
}
static ColumnPtr fill(const DataTypePtr& result_type,
Review Comment:
基础代码能否不修改
##########
be/src/exprs/function/function_variant_element_v2.h:
##########
@@ -0,0 +1,95 @@
+// 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.
+
+#pragma once
+
+#include <cstdint>
+#include <memory>
+#include <span>
+
+#include "common/status.h"
+#include "core/column/column.h"
+#include "core/string_ref.h"
+
+namespace doris {
+
+class ColumnVariantV2;
+
+class VariantElementV2PathSegment {
Review Comment:
be/src/exprs/function下对于Variant的文件拆分比较碎， 按功能整合一下，文件名更清晰
##########
be/src/exprs/aggregate/aggregate_function_window.h:
##########
@@ -421,9 +421,14 @@ template <bool arg_is_nullable>
struct BaseValue : public Value<arg_is_nullable> {
public:
bool is_null() const { return this->_ptr == nullptr; }
- // because _ptr pointer to first_argument or third argument, so it's
difficult to cast ptr
- // so here will call virtual function
- StringRef get_value() const { return
this->_ptr->get_data_at(this->_offset); }
+
+ void insert_into(IColumn& to) const {
Review Comment:
compute v2 需要对explode测试补充，覆盖对应的修改，
窗口函数相关的修改（be/src/exprs/aggregate/aggregate_function_window.h） 可以回退，这一期先不支持窗口函数
##########
be/src/core/column/variant_v2/column_variant_v2.cpp:
##########
@@ -0,0 +1,1604 @@
+// Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
把column_variant_v2*文件都挪到variant_v2目录下
##########
regression-test/suites/variant_p0/variant_compute_v2.groovy:
##########
@@ -0,0 +1,840 @@
+// 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.
+
+suite("variant_compute_v2") {
Review Comment:
在梳理下算子的测试， 是否覆盖多种primitive、object、array的variant， 或者混合类型， primitive的语义比如1，
1.0， decimal(1.0) 等等的测试，保证算子的正确性
--
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]
