JackieTien97 commented on code in PR #16720:
URL: https://github.com/apache/iotdb/pull/16720#discussion_r2512594826
##########
integration-test/src/test/java/org/apache/iotdb/relational/it/schema/IoTDBTableIT.java:
##########
@@ -761,6 +761,74 @@ public void testConcurrentAutoCreateAndDropColumn() throws
Exception {
}
}
+ @Test
+ public void testTableObjectCheck() throws Exception {
+ try (final Connection connection =
+ EnvFactory.getEnv().getConnection(BaseEnv.TABLE_SQL_DIALECT);
+ final Statement statement = connection.createStatement()) {
+ statement.execute("create database db2");
+ statement.execute("use db2");
+ statement.execute("create table \".\" ()");
+
+ try {
+ statement.execute("alter table \".\" add column a object");
+ fail();
+ } catch (final SQLException e) {
+ Assert.assertEquals(
+ "701: When there are object fields, the tableName . shall not be
'.', '..' or contain './', '.\\'",
+ e.getMessage());
+ }
Review Comment:
also need alter table statement whose table name is correct but column name
is not correct.
##########
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/impl/schema/table/AddTableColumnProcedure.java:
##########
@@ -77,6 +84,10 @@ protected Flow executeFromState(final ConfigNodeProcedureEnv
env, final AddTable
case PRE_RELEASE:
LOGGER.info("Pre release info of table {}.{} when adding column",
database, tableName);
preRelease(env);
+ if (table.setNeedCheck4Object()) {
+ checkObject(env, database, tableName);
+ }
Review Comment:
Actually, there is no need to do this check, if the table has already owned
another object type column before.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/schemaengine/table/DataNodeTableCache.java:
##########
@@ -110,6 +116,18 @@ public void init(final byte[] tableInitializationBytes) {
preUpdateTableMap.put(
PathUtils.unQualifyDatabaseName(key),
value.stream()
+ .filter(
Review Comment:
same as above. If you really want to this, you at least add some comments
about this.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/schemaengine/table/DataNodeTableCache.java:
##########
@@ -99,6 +100,11 @@ public void init(final byte[] tableInitializationBytes) {
databaseTableMap.put(
PathUtils.unQualifyDatabaseName(key),
value.stream()
+ .filter(
Review Comment:
use filter to do for-each style is not a good habit. You can do another
for-each out of the databaseTableMap.put
##########
integration-test/src/test/java/org/apache/iotdb/relational/it/schema/IoTDBTableIT.java:
##########
@@ -761,6 +761,74 @@ public void testConcurrentAutoCreateAndDropColumn() throws
Exception {
}
}
+ @Test
+ public void testTableObjectCheck() throws Exception {
Review Comment:
you need to construct a list which contains all illegal paths and then use
for-loop to format each sql
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/config/executor/ClusterConfigTaskExecutor.java:
##########
@@ -4273,6 +4273,12 @@ public SettableFuture<ConfigTaskResult>
alterTableAddColumn(
|| (TSStatusCode.COLUMN_ALREADY_EXISTS.getStatusCode() ==
tsStatus.getCode()
&& columnIfExists)) {
future.set(new ConfigTaskResult(TSStatusCode.SUCCESS_STATUS));
+ } else if (tsStatus.getMessage().contains("When there are object
fields")) {
+ future.setException(
+ new SemanticException(
+ tsStatus
+ .getMessage()
+
.replace("org.apache.iotdb.commons.exception.IoTDBRuntimeException: ", "")));
Review Comment:
don't use error msg to do this check, use the specific status code to do
this check
--
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]
