CRZbulabula commented on code in PR #17738:
URL: https://github.com/apache/iotdb/pull/17738#discussion_r3280191814
##########
integration-test/src/test/java/org/apache/iotdb/relational/it/schema/IoTDBDatabaseIT.java:
##########
@@ -636,13 +646,37 @@ public void testInformationSchema() throws SQLException {
statement.execute(
"CREATE VIEW test.view_table (tag1 STRING TAG,tag2 STRING TAG,s11
INT32 FIELD,s3 INT32 FIELD FROM s2) RESTRICT WITH (ttl=100) AS root.\"a\".**");
- TestUtils.assertResultSetEqual(
- statement.executeQuery("select * from databases"),
-
"database,ttl(ms),schema_replication_factor,data_replication_factor,time_partition_interval,schema_region_group_num,data_region_group_num,",
- new HashSet<>(
- Arrays.asList(
- "information_schema,INF,null,null,null,null,null,",
- "test,INF,1,1,604800000,0,0,")));
+ try (final ResultSet resultSet = statement.executeQuery("select * from
databases")) {
+ final ResultSetMetaData metaData = resultSet.getMetaData();
+ assertEquals(11, metaData.getColumnCount());
Review Comment:
This block only spot-checks the names of 4 new columns (at indexes 7, 8, 10,
11). If a future PR reorders any of the unchanged columns (1-6, 9), this test
will not catch it. Consider iterating over
`getSchemaTables().get(DATABASES).getColumnList()` (similar to how
`testManageDatabase` at lines 147-150 verifies the full header via
`showDBDetailsColumnHeaders`) to cover all columns.
##########
integration-test/src/test/java/org/apache/iotdb/relational/it/schema/IoTDBDatabaseIT.java:
##########
@@ -138,7 +138,9 @@ public void testManageDatabase() {
}
final int[] schemaRegionGroupNum = new int[] {0};
+ final int[] minSchemaRegionGroupNum = new int[] {1};
Review Comment:
The literal `1` here (and `2` on line 143) come from
`ConfigNodeConfig.defaultSchemaRegionGroupNumPerDatabase` /
`defaultDataRegionGroupNumPerDatabase`, but that's not obvious from the test.
Please add a brief comment like `// matches the cluster defaults from
ConfigNodeConfig (1 / 2)` so future maintainers don't have to chase down where
these magic numbers come from. Without this, the test would silently break if
anyone tweaks the defaults via `MppCommonConfig`.
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/schema/table/InformationSchema.java:
##########
@@ -106,9 +106,21 @@ public class InformationSchema {
databaseTable.addColumnSchema(
new AttributeColumnSchema(
ColumnHeaderConstant.SCHEMA_REGION_GROUP_NUM_TABLE_MODEL,
TSDataType.INT32));
+ databaseTable.addColumnSchema(
+ new AttributeColumnSchema(
+ ColumnHeaderConstant.MIN_SCHEMA_REGION_GROUP_NUM_TABLE_MODEL,
TSDataType.INT32));
Review Comment:
**Breaking change risk:** Inserting `min_schema_region_group_num` here (and
the three other new columns at lines 114, 120, 123) shifts the ordinal
positions of `data_region_group_num` and any column after it. Any client doing
positional access on `SELECT * FROM information_schema.databases` (e.g.
`resultSet.getInt(7)` to read what was `data_region_group_num`) will silently
read `min_schema_region_group_num` instead.
If positional stability is important, consider appending the four new
columns at the end of the table definition. Otherwise, please flag this schema
change in the PR description / release notes so downstream consumers can adjust.
##########
integration-test/src/test/java/org/apache/iotdb/relational/it/schema/IoTDBDatabaseIT.java:
##########
@@ -636,13 +646,37 @@ public void testInformationSchema() throws SQLException {
statement.execute(
"CREATE VIEW test.view_table (tag1 STRING TAG,tag2 STRING TAG,s11
INT32 FIELD,s3 INT32 FIELD FROM s2) RESTRICT WITH (ttl=100) AS root.\"a\".**");
- TestUtils.assertResultSetEqual(
- statement.executeQuery("select * from databases"),
-
"database,ttl(ms),schema_replication_factor,data_replication_factor,time_partition_interval,schema_region_group_num,data_region_group_num,",
- new HashSet<>(
- Arrays.asList(
- "information_schema,INF,null,null,null,null,null,",
- "test,INF,1,1,604800000,0,0,")));
+ try (final ResultSet resultSet = statement.executeQuery("select * from
databases")) {
+ final ResultSetMetaData metaData = resultSet.getMetaData();
+ assertEquals(11, metaData.getColumnCount());
+ assertEquals("min_schema_region_group_num", metaData.getColumnName(7));
+ assertEquals("max_schema_region_group_num", metaData.getColumnName(8));
+ assertEquals("min_data_region_group_num", metaData.getColumnName(10));
+ assertEquals("max_data_region_group_num", metaData.getColumnName(11));
+
+ int cnt = 0;
+ while (resultSet.next()) {
+ if ("information_schema".equals(resultSet.getString(1))) {
+ for (int columnIndex = 3; columnIndex <= 11; columnIndex++) {
+ Assert.assertNull(resultSet.getObject(columnIndex));
Review Comment:
Style nit: this file already statically imports `assertEquals`,
`assertTrue`, `assertFalse`, and `fail` (lines 48-51). For consistency, please
add `import static org.junit.Assert.assertNull;` and use
`assertNull(resultSet.getObject(columnIndex))` directly instead of the
qualified `Assert.assertNull(...)`.
--
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]
