Change system table check logic #36502

makssent · 2025-09-08T14:44:07Z

Fixes #36462

Changes proposed in this pull request:

Moved system-table detection to SystemTableManager and relocated it under shardingsphere/database/core.
Introduced a dialect-driven SPI: DialectSystemTableManager to provide per-dialect system table lists and checks.
Updated SchemaMetaDataLoader to delegate to the dialect implementation’s isSystemTable, falling back to the legacy heuristic when no dialect is available.
Added unit tests.

Before committing this PR, I'm sure that I have checked the following options:

My code follows the code of conduct of this project.
I have self-reviewed the commit code.
I have (or in comment I request) added corresponding labels for the pull request.
I have passed maven check locally : ./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.
I have made corresponding changes to the documentation.
I have added corresponding unit tests for my changes.
I have updated the Release Notes of the current development version. For more details, see Update Release Note

makssent · 2025-09-08T16:39:38Z

There are two points I’m not fully satisfied with:

In SystemTableManagerTest I had to implement a custom schema loading mechanism, because the schemas are not available during tests before the program starts. At runtime everything works correctly, the issue only exists in the test environment. I couldn’t find another solution: adding dialect-connector-* dependencies would be a more natural option, but it breaks the build.
The TODOs I left. I think they should probably be removed, but I wanted to highlight one point: in SimpleTableSegmentBinder we currently use SystemTableManager.isSystemTable() without databaseType, even though databaseType is available and this would not change the program logic. The code could be rewritten like this, and the implementation of isSystemTable without databaseType could be removed from SystemTableManager:
```
if (SystemTableManager.isSystemTable(binderContext.getSqlStatement().getDatabaseType().getType(),
        schemaName, tableName)) {
    return;
}
```
This would make the method that only accepts schema unnecessary, since it’s not used anywhere else. I didn’t remove it in this PR — maybe you have future plans for it — but I wanted to draw attention to this point.

I verified it with tests. I also checked Firebird manually — everything works as intended.

Looking forward to your review and suggestions for improvement.

strongduanmu

Finally, I would like to remind you that to determine whether a table is a true system table, you also need to look at the path to which the table belongs (db - schema - table). If the path belongs to a user-defined schema, perhaps this table should be identified as a non-system table.

strongduanmu · 2025-09-10T09:49:50Z

...pache/shardingsphere/database/connector/core/metadata/manager/DialectSystemTableManager.java

+ * Dialect system table manager.
+ */
+@SingletonSPI
+public interface DialectSystemTableManager extends DatabaseTypedSPI {


Do you think DialectSystemTableDetector is better? The meaning of Manager is broader, and we only want to add a class to detect whether a table is a system table.

strongduanmu · 2025-09-10T09:52:15Z

...e/shardingsphere/database/connector/core/metadata/data/loader/type/SchemaMetaDataLoader.java

@@ -124,6 +126,7 @@ private Collection<String> loadTableNames(final Connection connection, final Str
    }

    private boolean isSystemTable(final String table) {
-        return table.contains("$") || table.contains("/") || table.contains("##");
+        return DatabaseTypedSPILoader.findService(DialectSystemTableManager.class, databaseType)


Maybe you can add a engine class to hide the details of SPI loading so that other scenarios can directly call the Engine. You can refer to the logic of SQLRouteEngine.

strongduanmu · 2025-09-10T09:54:00Z

...a/org/apache/shardingsphere/database/connector/core/metadata/manager/SystemTableManager.java

+ * System table manager.
+ */
+@NoArgsConstructor(access = AccessLevel.PRIVATE)
+public final class SystemTableManager {


SystemTableDetector maybe better.

strongduanmu · 2025-09-10T09:54:45Z

...a/org/apache/shardingsphere/database/connector/core/metadata/manager/SystemTableManager.java

+        try (Stream<String> resourceNameStream = ClasspathResourceDirectoryReader.read("schema")) {
+            resourceNames = resourceNameStream.filter(each -> each.endsWith(".yaml")).collect(Collectors.toList());
+        }
+        DATABASE_TYPE_SCHEMA_TABLE_MAP = resourceNames.stream().map(resourceName -> resourceName.split("/")).filter(each -> each.length == 4)


Please put 4 costant to the left hand of each.length == 4

strongduanmu · 2025-09-10T09:55:14Z

...a/org/apache/shardingsphere/database/connector/core/metadata/manager/SystemTableManager.java

+     */
+    // TODO check if this function is needed, since it's only used in SimpleTableSegmentBinder and could be replaced by SystemTableManager.isSystemTable(databaseType). (issues/36462)
+    public static boolean isSystemTable(final String schema, final String tableName) {
+        for (Map.Entry<String, Map<String, Collection<String>>> entry : DATABASE_TYPE_SCHEMA_TABLE_MAP.entrySet()) {


Please use Entry to replace Map.Entry

strongduanmu · 2025-09-10T09:57:03Z

...a/org/apache/shardingsphere/database/connector/core/metadata/manager/SystemTableManager.java

+     * @param tableName    table name
+     * @return whether current table is system table or not
+     */
+    public static boolean isSystemTable(final String databaseType, final String schema, final String tableName) {


These logics seem very common. As long as the system table is configured under resource, they can work normally?

If this is the case, perhaps we can refactor it without SPI, as long as we ensure that the database without configuring the system table YAML file can go to the default logic.

strongduanmu · 2025-09-10T10:02:50Z

...g/apache/shardingsphere/database/connector/core/metadata/manager/SystemTableManagerTest.java

+    static void setUp() throws Exception {
+        originalClassLoader = Thread.currentThread().getContextClassLoader();
+        URL[] urls = {
+                Paths.get("../dialect/mysql/src/main/resources").toUri().toURL(),


You can write some FIXTUREs and test configurations specifically for testing to test the logic of SystemTableManager without actually loading the dialect module.

strongduanmu · 2025-09-10T10:03:56Z

.../shardingsphere/database/connector/firebird/metadata/manager/FirebirdSystemTableManager.java

+public final class FirebirdSystemTableManager implements DialectSystemTableManager {
+
+    @Override
+    public String getDatabaseType() {


It seems that the implementation classes of these SPIs are empty. Based on this, I suggest removing the SPI interface and implementation class and encapsulating these logics through a SystemTableDetector.

makssent · 2025-09-14T10:17:06Z

Hi, sorry for the late reply — I’m working on another task in parallel. I’ve reviewed your comments and will apply the fixes. Thanks!

eliminate redundant isSystemTable overload, remove unused getTables method, rework SPI from Manager to Rule

makssent · 2025-09-14T14:33:56Z

I hope I’ve finished and done everything correctly. I think my problem was also that I just copied the old implementation from SystemSchemaManager and didn’t even try to change it, trying not to deviate too much from the base code, not assuming that it was already outdated and not fully aligned with current standards.

I created a new SPI — SystemTableRule (now optional, so each database can define its own rules). I removed the unnecessary override of isSystemTable() because everywhere it is used we already know the databaseType. I also deleted the unused getTables() function, renamed *Manager to *Detector, and did some refactoring. I think it will be better to implement specific logic later, when it is actually needed, instead of keeping it around without use.

I’m looking forward to your next comments, and I hope I didn’t miss anything. It was interesting for me to dig into the core.

makssent · 2025-09-14T14:36:04Z

I see that the build is failing, but I noticed the same error appears in other PRs as well. I’ll wait until the issue is resolved on your side and then rerun the CI.

makssent added 2 commits September 8, 2025 17:32

Change system table check logic in SchemaMetaDataLoader

5a1855c

fix test

ad2258e

makssent closed this Sep 8, 2025

makssent reopened this Sep 8, 2025

makssent marked this pull request as ready for review September 8, 2025 16:54

strongduanmu requested changes Sep 10, 2025

View reviewed changes

makssent marked this pull request as draft September 14, 2025 13:06

makssent added 5 commits September 14, 2025 16:18

add fixtures for SystemTableManager without dialect loading

61262d1

eliminate unnecessary SPI from SystemTableManager

95eccb9

add Engine to hide SPI loading for SystemTableManager

45b9610

rename *Manager to *Detector,

b55e2b0

eliminate redundant isSystemTable overload, remove unused getTables method, rework SPI from Manager to Rule

Merge branch 'master' into master_226041_system_tables_filter_fix

110ea80

makssent marked this pull request as ready for review September 14, 2025 14:35

makssent requested a review from strongduanmu September 14, 2025 14:38

ci

763dbff

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Change system table check logic #36502

Change system table check logic #36502

Uh oh!

makssent commented Sep 8, 2025

Uh oh!

makssent commented Sep 8, 2025

Uh oh!

strongduanmu left a comment

Uh oh!

strongduanmu Sep 10, 2025

Uh oh!

strongduanmu Sep 10, 2025

Uh oh!

strongduanmu Sep 10, 2025

Uh oh!

strongduanmu Sep 10, 2025

Uh oh!

strongduanmu Sep 10, 2025

Uh oh!

strongduanmu Sep 10, 2025

Uh oh!

strongduanmu Sep 10, 2025

Uh oh!

strongduanmu Sep 10, 2025

Uh oh!

strongduanmu Sep 10, 2025

Uh oh!

makssent commented Sep 14, 2025

Uh oh!

makssent commented Sep 14, 2025 •

edited

Loading

Uh oh!

makssent commented Sep 14, 2025

Uh oh!

Uh oh!

Change system table check logic #36502

Are you sure you want to change the base?

Change system table check logic #36502

Uh oh!

Conversation

makssent commented Sep 8, 2025

Uh oh!

makssent commented Sep 8, 2025

Uh oh!

strongduanmu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

makssent commented Sep 14, 2025

Uh oh!

makssent commented Sep 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

makssent commented Sep 14, 2025

Uh oh!

Uh oh!

makssent commented Sep 14, 2025 •

edited

Loading