github-actions[bot] commented on code in PR #68604: URL: https://github.com/apache/doris/pull/68604#discussion_r4129342819
########## be/test/format/arrow/arrow_flight_timestamp_test.cpp: ########## @@ -0,0 +1,212 @@ +// 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 <arrow/api.h> +#include <gtest/gtest.h> + +#include "core/column/column_nullable.h" +#include "core/column/column_vector.h" +#include "core/data_type/data_type_array.h" +#include "core/data_type/data_type_date_or_datetime_v2.h" +#include "core/data_type/data_type_map.h" +#include "core/data_type/data_type_nullable.h" +#include "core/data_type/data_type_struct.h" +#include "format/arrow/arrow_block_convertor.h" +#include "util/timezone_utils.h" + +namespace doris { +namespace { +using DateTime = DateV2Value<DateTimeV2ValueType>; + +Block timestamp_block(int scale, const std::vector<DateTime>& values) { + auto column = ColumnDateTimeV2::create(); + for (const auto& value : values) { + column->insert_value(value); + } + return Block {{std::move(column), std::make_shared<DataTypeDateTimeV2>(scale), "event_time"}}; +} + +class ArrowFlightTimestampTest : public testing::Test { +protected: + static void SetUpTestSuite() { TimezoneUtils::load_timezones_to_cache(); } +}; + +TEST_F(ArrowFlightTimestampTest, RejectsOutOfRangeInEveryUnitWithoutPublishingBatch) { + for (int scale : {0, 3, 6}) { + for (const auto& value : + {DateTime {}, DateTime(0, 12, 31, 0, 0, 0, 0), DateTime(10000, 1, 1, 0, 0, 0, 0)}) { Review Comment: `DateTime` aliases `DateV2Value<DateTimeV2ValueType>`, but its seven-argument constructor is private in `vdatetime_value.h`; the public constructors take zero or one argument. This call and the later date literals therefore fail compilation, and `be/test/CMakeLists.txt` includes this file in the BE unit-test target. Please construct these fixtures through the public date API so the new tests compile. ########## regression-test/suites/arrow_flight_sql_p0/test_flight_int96_timestamp_range.groovy: ########## @@ -0,0 +1,40 @@ +// 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_flight_int96_timestamp_range", "arrow_flight_sql,external,hive,tvf,external_docker") { + if (!"true".equalsIgnoreCase(context.config.otherConfigs.get("enableHiveTest"))) { + return + } + def host = context.config.otherConfigs.get("externalEnvIp") + def port = context.config.otherConfigs.get("hive2HdfsPort") + def flightUrl = context.getArrowFlightSqlConnection().getMetaData().getURL() + connect(context.config.jdbcUser, context.config.jdbcPassword, flightUrl) { + // Exercise native INT96 materialization, including legacy zero-date compatibility. + for (def file : ["part-00000-570d8e52-652d-4892-8bdc-7fa5466ffa69.c000.snappy.parquet", Review Comment: Four files in this negative loop have existing group4 one-row results with timestamps no later than `9999-12-31T23:59:59.999999` (tests 75, 158, 236 and 314). Those goldens were updated when raw INT96 began mapping to timezone-naive DATETIMEV2; with the TVF's empty INT96 timezone/UTC Flight binding, the validator accepts those values. Only `int96_timestamps_nanos_outside_day_range.parquet` has a year-0000 result. Requiring the range error for all five fails for the in-range fixtures; please keep the year-0000 file as the negative case and assert success for the others. ########## regression-test/suites/arrow_flight_sql_p0/test_flight_int96_timestamp_range.groovy: ########## @@ -0,0 +1,40 @@ +// 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_flight_int96_timestamp_range", "arrow_flight_sql,external,hive,tvf,external_docker") { + if (!"true".equalsIgnoreCase(context.config.otherConfigs.get("enableHiveTest"))) { + return + } + def host = context.config.otherConfigs.get("externalEnvIp") + def port = context.config.otherConfigs.get("hive2HdfsPort") + def flightUrl = context.getArrowFlightSqlConnection().getMetaData().getURL() + connect(context.config.jdbcUser, context.config.jdbcPassword, flightUrl) { Review Comment: With `enableTLS=true`, `SuiteContext.connect` returns a decorated URL before invoking its closure when the URL lacks `clientCertificateKeyStoreUrl`; the Flight URL built by `Config` lacks that parameter. All five assertions here are inside the skipped closure, so this suite can pass without testing the change. The scalar Flight suite uses the same pattern. Please route these assertions through a connection path that executes in TLS runs. ########## regression-test/suites/arrow_flight_sql_p0/test_flight_timestamp_range.groovy: ########## @@ -0,0 +1,82 @@ +// 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. + +import java.time.LocalDateTime + +suite("test_flight_timestamp_range", "arrow_flight_sql") { + sql "DROP TABLE IF EXISTS flight_timestamp_input" + sql """CREATE TABLE flight_timestamp_input (id INT, value STRING) + DUPLICATE KEY(id) DISTRIBUTED BY HASH(id) BUCKETS 1 + PROPERTIES ("replication_num" = "1")""" + sql """INSERT INTO flight_timestamp_input VALUES + (1, '0001-01-01 00:00:00.000000'), + (2, '9999-12-31 23:59:59.999999'), + (3, '1969-12-31 23:59:59.999999'), + (4, NULL), (5, '0000-12-31 00:00:00.000000')""" + try { + // Check that the source value reaches the date conversion instead of becoming SQL NULL. + assertEquals([['0000-12-31 00:00:00.000000']], + sql("SELECT CAST(CAST(value AS DATETIME(6)) AS STRING) FROM flight_timestamp_input WHERE id = 5")) + def flightUrl = context.getArrowFlightSqlConnection().getMetaData().getURL() + connect(context.config.jdbcUser, context.config.jdbcPassword, flightUrl) { Review Comment: The setup `sql` calls in this Flight suite use `getArrowFlightSqlConnection()`, authenticated with `extArrowFlightSqlUser`/`extArrowFlightSqlPassword`, but this `connect` runs the assertions and typed read as `jdbcUser`/`jdbcPassword`. With distinct valid accounts, the JDBC account can have CREATE on the suite DB but no SELECT on the table created by the Flight account, so the tests fail on access control before exercising timestamp validation. The INT96 suite reconnects the same way. Please use the configured Flight credentials for these assertions. -- 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]
