Add region group stats to table model database details#17738
Add region group stats to table model database details#17738zerolbsony wants to merge 2 commits into
Conversation
Expose min/max schema and data RegionGroup numbers in table-model SHOW DATABASES DETAILS and information_schema.databases, keeping the metadata view aligned with tree-model database detail output.
CRZbulabula
left a comment
CRZbulabula
left a comment
There was a problem hiding this comment.
Functional changes look correct — the column ordering is consistent across the three writers (ShowDBTask, InformationSchemaContentSupplierFactory, schema definition in InformationSchema), and the underlying getters on TDatabaseInfo already exist (used by tree-model ShowDatabaseStatement). Left a few inline comments — the main one to consider is the implicit breaking change on information_schema.databases column ordering.
| ColumnHeaderConstant.SCHEMA_REGION_GROUP_NUM_TABLE_MODEL, TSDataType.INT32)); | ||
| databaseTable.addColumnSchema( | ||
| new AttributeColumnSchema( | ||
| ColumnHeaderConstant.MIN_SCHEMA_REGION_GROUP_NUM_TABLE_MODEL, TSDataType.INT32)); |
There was a problem hiding this 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.
| } | ||
|
|
||
| final int[] schemaRegionGroupNum = new int[] {0}; | ||
| final int[] minSchemaRegionGroupNum = new int[] {1}; |
There was a problem hiding this 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.
| "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()); |
There was a problem hiding this 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.
| while (resultSet.next()) { | ||
| if ("information_schema".equals(resultSet.getString(1))) { | ||
| for (int columnIndex = 3; columnIndex <= 11; columnIndex++) { | ||
| Assert.assertNull(resultSet.getObject(columnIndex)); |
There was a problem hiding this 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(...).
2 participants
Expose min/max schema and data RegionGroup numbers in table-model SHOW DATABASES DETAILS and information_schema.databases, keeping the metadata view aligned with tree-model database detail output.
