Copilot commented on code in PR #61090:
URL: https://github.com/apache/doris/pull/61090#discussion_r2890927797
##########
be/src/vec/common/variant_util.cpp:
##########
@@ -958,6 +959,7 @@ Status VariantCompactionUtil::check_path_stats(const
std::vector<RowsetSharedPtr
column->name());
}
}
+#endif
Review Comment:
Wrapping the output-rowset extracted-column invariant check in `#ifndef
BE_TEST` makes unit-test builds skip a correctness validation that still runs
in production builds. This can mask real regressions (e.g. new extracted
columns like NestedGroup-related columns) and reduce the value of the tests.
Prefer keeping the check enabled and extending the allow-list/conditions
explicitly for any newly legitimate extracted columns, instead of disabling it
under BE_TEST.
##########
regression-test/suites/variant_p0/predefine/predefined_typed_to_sparse_1shard.groovy:
##########
@@ -0,0 +1,137 @@
+// 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("test_predefine_typed_to_sparse_1shard", "p0"){
+ sql """ set enable_common_expr_pushdown = true """
+ sql """ set default_variant_enable_doc_mode = false """
+ sql """ set enable_variant_schema_auto_cast = false """
+
+ def load_json_data = {table_name, file_name ->
+ // load the json data
+ streamLoad {
+ table "${table_name}"
+
+ // set http request header params
+ set 'read_json_by_line', 'true'
+ set 'format', 'json'
+ set 'max_filter_ratio', '0.1'
+ set 'memtable_on_sink_node', 'true'
+ file file_name // import json file
+ time 10000 // limit inflight 10s
+
+ // if declared a check callback, the default check condition will
ignore.
+ // So you must check all condition
+
+ check { result, exception, startTime, endTime ->
+ if (exception != null) {
+ throw exception
+ }
+ logger.info("Stream load ${file_name} result:
${result}".toString())
+ def json = parseJson(result)
+ assertEquals("success", json.Status.toLowerCase())
+ // assertEquals(json.NumberTotalRows, json.NumberLoadedRows +
json.NumberUnselectedRows)
+ assertTrue(json.NumberLoadedRows > 0 && json.LoadBytes > 0)
+ }
+ }
+ }
+
+ def tableName = "test_predefine_typed_to_sparse"
Review Comment:
This suite uses the same table name (`test_predefine_typed_to_sparse`) as
`predefined_typed_to_sparse.groovy`. If the regression runner executes suites
concurrently (or reuses the same DB between suites), the DROP/CREATE/INSERT
here can race and cause flaky failures. Please use a table name unique to this
suite (e.g. include `_1shard`).
```suggestion
def tableName = "test_predefine_typed_to_sparse_1shard"
```
##########
regression-test/suites/variant_p0/predefine/predefined_typed_to_sparse_1shard.groovy:
##########
@@ -0,0 +1,137 @@
+// 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("test_predefine_typed_to_sparse_1shard", "p0"){
+ sql """ set enable_common_expr_pushdown = true """
+ sql """ set default_variant_enable_doc_mode = false """
+ sql """ set enable_variant_schema_auto_cast = false """
+
+ def load_json_data = {table_name, file_name ->
+ // load the json data
+ streamLoad {
+ table "${table_name}"
+
+ // set http request header params
+ set 'read_json_by_line', 'true'
+ set 'format', 'json'
+ set 'max_filter_ratio', '0.1'
+ set 'memtable_on_sink_node', 'true'
+ file file_name // import json file
+ time 10000 // limit inflight 10s
+
+ // if declared a check callback, the default check condition will
ignore.
+ // So you must check all condition
+
+ check { result, exception, startTime, endTime ->
+ if (exception != null) {
+ throw exception
+ }
+ logger.info("Stream load ${file_name} result:
${result}".toString())
+ def json = parseJson(result)
+ assertEquals("success", json.Status.toLowerCase())
+ // assertEquals(json.NumberTotalRows, json.NumberLoadedRows +
json.NumberUnselectedRows)
+ assertTrue(json.NumberLoadedRows > 0 && json.LoadBytes > 0)
+ }
+ }
+ }
+
Review Comment:
`load_json_data` closure is defined but never used in this suite. Keeping
unused streamLoad logic makes the test harder to read and maintain; please
remove it or use it for data loading (as in
`predefined_typed_to_sparse.groovy`).
```suggestion
```
--
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]
