Copilot commented on code in PR #59387:
URL: https://github.com/apache/doris/pull/59387#discussion_r2660494037
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalTable.java:
##########
@@ -276,16 +276,14 @@ public long getCreateTime() {
return 0;
}
- // return schema update time as default
- // override this method if there is some other kinds of update time
- // use getSchemaUpdateTime if just need the schema update time
@Override
+ // get the max value of `schemaUpdateTime` and `eventUpdateTime` as
`updateTime`
Review Comment:
The comment is now outdated. Since `schemaUpdateTime` and `eventUpdateTime`
have been merged into a single `updateTime` field, this comment should be
updated to simply describe that this method returns the table update time,
which tracks when the table was last modified (e.g., by insert, alter, or
refresh operations).
```suggestion
// Returns the table update time, tracking when the table was last
modified
// (for example, by insert, alter, or refresh operations).
```
##########
regression-test/suites/external_table_p0/test_external_table_update_time.groovy:
##########
@@ -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.
+
+suite("test_external_table_update_time",
"p0,external,hive,external_docker,external_docker_hive") {
+ String enabled = context.config.otherConfigs.get("enableHiveTest")
+ if (enabled == null || !enabled.equalsIgnoreCase("true")) {
+ logger.info("disable Hive test.")
+ return
+ }
+
+ for (String hivePrefix : ["hive3"]) {
+ String extHiveHmsHost =
context.config.otherConfigs.get("externalEnvIp")
+ String extHiveHmsPort = context.config.otherConfigs.get(hivePrefix +
"HmsPort")
+ String catalog_name = hivePrefix + "_test_update_time_ctl"
+ sql """drop catalog if exists ${catalog_name};"""
+ sql """
+ create catalog if not exists ${catalog_name} properties (
+ 'type'='hms',
+ 'hadoop.username' = 'hadoop',
+ 'hive.metastore.uris' =
'thrift://${extHiveHmsHost}:${extHiveHmsPort}'
+ );
+ """
+ logger.info("catalog " + catalog_name + " created")
+ sql """switch ${catalog_name};"""
+
+ sql "drop database if exists test_update_time_db force";
+ sql "create database test_update_time_db";
+ sql "use test_update_time_db";
+ sql "create table test_update_time_tbl(k1 int)"
+ sql "insert into test_update_time_tbl values(1)"
+ def result = sql """select UPDATE_TIME from information_schema.tables
where TABLE_SCHEMA="test_update_time_db" and
TABLE_NAME="test_update_time_tbl""""
+ def update_time1 = result[0][0];
+ sleep(2000);
+ sql "insert into test_update_time_tbl values(2)"
+ result = sql """select UPDATE_TIME from information_schema.tables
where TABLE_SCHEMA="test_update_time_db" and
TABLE_NAME="test_update_time_tbl""""
+ def update_time2 = result[0][0];
+ logger.info("get update times " + update_time1 + " vs. " +
update_time2)
Review Comment:
There is a trailing whitespace at the end of this line that should be
removed.
```suggestion
logger.info("get update times " + update_time1 + " vs. " +
update_time2)
```
##########
regression-test/suites/external_table_p0/iceberg/iceberg_branch_tag_operate.groovy:
##########
@@ -53,6 +53,9 @@ suite("iceberg_branch_tag_operate",
"p0,external,doris,external_docker,external_
sql """ alter table test_branch_tag_operate create branch b1 """
sql """ alter table test_branch_tag_operate create branch if not exists b1
"""
+ def result = sql """select UPDATE_TIME from information_schema.tables
where TABLE_SCHEMA="test_db" and TABLE_NAME="test_branch_tag_operate""""
Review Comment:
There is a trailing whitespace at the end of this line that should be
removed.
```suggestion
def result = sql """select UPDATE_TIME from information_schema.tables
where TABLE_SCHEMA="test_db" and TABLE_NAME="test_branch_tag_operate""""
```
##########
regression-test/suites/external_table_p0/iceberg/iceberg_branch_tag_operate.groovy:
##########
@@ -137,6 +144,10 @@ suite("iceberg_branch_tag_operate",
"p0,external,doris,external_docker,external_
exception "Cannot set b8 to unknown snapshot: 11223344"
}
+ result = sql """select UPDATE_TIME from information_schema.tables where
TABLE_SCHEMA="test_db" and TABLE_NAME="test_branch_tag_operate""""
Review Comment:
There is a trailing whitespace at the end of this line that should be
removed.
```suggestion
result = sql """select UPDATE_TIME from information_schema.tables where
TABLE_SCHEMA="test_db" and TABLE_NAME="test_branch_tag_operate""""
```
##########
regression-test/suites/external_table_p0/iceberg/iceberg_branch_tag_operate.groovy:
##########
@@ -72,7 +75,11 @@ suite("iceberg_branch_tag_operate",
"p0,external,doris,external_docker,external_
sql """ insert into test_branch_tag_operate values (3) """
sql """ insert into test_branch_tag_operate values (4) """
sql """ insert into test_branch_tag_operate values (5) """
-
+ result = sql """select UPDATE_TIME from information_schema.tables where
TABLE_SCHEMA="test_db" and TABLE_NAME="test_branch_tag_operate""""
Review Comment:
There is a trailing whitespace at the end of this line that should be
removed.
##########
regression-test/suites/external_table_p0/test_external_table_update_time.groovy:
##########
@@ -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.
+
+suite("test_external_table_update_time",
"p0,external,hive,external_docker,external_docker_hive") {
+ String enabled = context.config.otherConfigs.get("enableHiveTest")
+ if (enabled == null || !enabled.equalsIgnoreCase("true")) {
+ logger.info("disable Hive test.")
+ return
+ }
+
+ for (String hivePrefix : ["hive3"]) {
+ String extHiveHmsHost =
context.config.otherConfigs.get("externalEnvIp")
+ String extHiveHmsPort = context.config.otherConfigs.get(hivePrefix +
"HmsPort")
+ String catalog_name = hivePrefix + "_test_update_time_ctl"
+ sql """drop catalog if exists ${catalog_name};"""
+ sql """
+ create catalog if not exists ${catalog_name} properties (
+ 'type'='hms',
+ 'hadoop.username' = 'hadoop',
+ 'hive.metastore.uris' =
'thrift://${extHiveHmsHost}:${extHiveHmsPort}'
+ );
+ """
+ logger.info("catalog " + catalog_name + " created")
+ sql """switch ${catalog_name};"""
+
+ sql "drop database if exists test_update_time_db force";
+ sql "create database test_update_time_db";
+ sql "use test_update_time_db";
+ sql "create table test_update_time_tbl(k1 int)"
+ sql "insert into test_update_time_tbl values(1)"
+ def result = sql """select UPDATE_TIME from information_schema.tables
where TABLE_SCHEMA="test_update_time_db" and
TABLE_NAME="test_update_time_tbl""""
+ def update_time1 = result[0][0];
+ sleep(2000);
+ sql "insert into test_update_time_tbl values(2)"
+ result = sql """select UPDATE_TIME from information_schema.tables
where TABLE_SCHEMA="test_update_time_db" and
TABLE_NAME="test_update_time_tbl""""
+ def update_time2 = result[0][0];
+ logger.info("get update times " + update_time1 + " vs. " +
update_time2)
+ assertTrue(update_time2 > update_time1);
+ sleep(2000);
+ sql "truncate table test_update_time_tbl";
+ result = sql """select UPDATE_TIME from information_schema.tables
where TABLE_SCHEMA="test_update_time_db" and
TABLE_NAME="test_update_time_tbl""""
+ def update_time3 = result[0][0];
+ logger.info("get update times " + update_time2 + " vs. " +
update_time3)
+ assertTrue(update_time3 > update_time2);
Review Comment:
The test does not clean up the database and catalog it creates. Consider
adding cleanup code after the assertions to drop the test database and catalog
to prevent test environment pollution.
```suggestion
try {
sql "drop database if exists test_update_time_db force";
sql "create database test_update_time_db";
sql "use test_update_time_db";
sql "create table test_update_time_tbl(k1 int)"
sql "insert into test_update_time_tbl values(1)"
def result = sql """select UPDATE_TIME from
information_schema.tables where TABLE_SCHEMA="test_update_time_db" and
TABLE_NAME="test_update_time_tbl""""
def update_time1 = result[0][0];
sleep(2000);
sql "insert into test_update_time_tbl values(2)"
result = sql """select UPDATE_TIME from
information_schema.tables where TABLE_SCHEMA="test_update_time_db" and
TABLE_NAME="test_update_time_tbl""""
def update_time2 = result[0][0];
logger.info("get update times " + update_time1 + " vs. " +
update_time2)
assertTrue(update_time2 > update_time1);
sleep(2000);
sql "truncate table test_update_time_tbl";
result = sql """select UPDATE_TIME from
information_schema.tables where TABLE_SCHEMA="test_update_time_db" and
TABLE_NAME="test_update_time_tbl""""
def update_time3 = result[0][0];
logger.info("get update times " + update_time2 + " vs. " +
update_time3)
assertTrue(update_time3 > update_time2);
} finally {
// Cleanup test objects to avoid polluting the test environment
try {
sql "drop table if exists test_update_time_tbl";
} catch (Throwable t) {
logger.warn("Failed to drop table test_update_time_tbl
during cleanup", t)
}
try {
sql "drop database if exists test_update_time_db force";
} catch (Throwable t) {
logger.warn("Failed to drop database test_update_time_db
during cleanup", t)
}
try {
sql "switch internal";
sql """drop catalog if exists ${catalog_name};"""
} catch (Throwable t) {
logger.warn("Failed to drop catalog ${catalog_name} during
cleanup", t)
}
}
```
--
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]
