[GitHub] [shardingsphere] tristaZero commented on a change in pull request #10840: Improve the initialization of OptimizeContextFactory for issue#10678

[GitBox <gi...@apache.org>]

tristaZero commented on a change in pull request #10840:
URL: https://github.com/apache/shardingsphere/pull/10840#discussion_r655079971



##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/test/java/org/apache/shardingsphere/infra/metadata/schema/builder/SchemaBuilderTest.java
##########
@@ -47,19 +48,19 @@
 
 @RunWith(MockitoJUnitRunner.class)
 public final class SchemaBuilderTest {
-    
+
     private static final String TEST_CATALOG = "catalog";
-    

Review comment:
       Hi, please keep indent each line here.

##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/metadata/schema/builder/SchemaBuilder.java
##########
@@ -126,38 +144,38 @@ private static void appendDialectRemainTables(final DialectTableMetaDataLoader d
             }
         }
     }
-    
-    private static void appendDefaultRemainTables(final SchemaBuilderMaterials materials, final ShardingSphereSchema schema) throws SQLException {
-        Collection<String> existedTableNames = getExistedTables(materials.getRules(), schema);
-        for (Entry<String, DataSource> entry : materials.getDataSourceMap().entrySet()) {
+
+    private static void appendDefaultRemainTables(final SchemaBuilderMaterials materials, final Map<String, TableMetaData> tables) throws SQLException {
+        Collection<String> existedTableNames = getExistedTables(materials.getRules(), tables);
+        for (Map.Entry<String, DataSource> entry : materials.getDataSourceMap().entrySet()) {

Review comment:
       Please import `Map.Entry` to avoid `Map.`

##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/metadata/schema/builder/SchemaBuilder.java
##########
@@ -64,42 +63,61 @@
     static {
         ShardingSphereServiceLoader.register(DialectTableMetaDataLoader.class);
     }
-    
+
     /**
-     * Build ShardingSphere schema.
-     * 
+     * build actual and logic table meta data.
+     *
      * @param materials schema builder materials
-     * @return ShardingSphere schema
+     * @return actual and logic table meta data
      * @throws SQLException SQL exception
      */
-    public static ShardingSphereSchema build(final SchemaBuilderMaterials materials) throws SQLException {
-        ShardingSphereSchema result = new ShardingSphereSchema();
-        addRuleConfiguredTables(materials, result);
-        appendRemainTables(materials, result);
+    public static Map<Map<String, TableMetaData>, Map<String, TableMetaData>> build(final SchemaBuilderMaterials materials) throws SQLException {

Review comment:
       `Map<Map<String, TableMetaData>, Map<String, TableMetaData>>` seems a little wired, maybe we need a table Name field in `TableMetaData`? How do you think?




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org