You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by GitBox <gi...@apache.org> on 2021/04/07 08:19:21 UTC
[GitHub] [shardingsphere] MingxingLAI opened a new pull request #9967: Issue#9948 refactor TablesContext
MingxingLAI opened a new pull request #9967:
URL: https://github.com/apache/shardingsphere/pull/9967
Fixes #9948 .
Changes proposed in this pull request:
1. getTableNames function
2. TablesContext constructor
--
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
[GitHub] [shardingsphere] MingxingLAI commented on pull request #9967: Issue#9948 refactor TablesContext
Posted by GitBox <gi...@apache.org>.
MingxingLAI commented on pull request #9967:
URL: https://github.com/apache/shardingsphere/pull/9967#issuecomment-824505771
I think it has been explained, and ran all the unit tests, you can help me look at the code.
--
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
[GitHub] [shardingsphere] tristaZero commented on a change in pull request #9967: Issue#9948 refactor TablesContext
Posted by GitBox <gi...@apache.org>.
tristaZero commented on a change in pull request #9967:
URL: https://github.com/apache/shardingsphere/pull/9967#discussion_r617346309
##########
File path: shardingsphere-infra/shardingsphere-infra-binder/src/main/java/org/apache/shardingsphere/infra/binder/segment/table/TablesContext.java
##########
@@ -46,14 +46,9 @@ public TablesContext(final SimpleTableSegment tableSegment) {
}
public TablesContext(final Collection<SimpleTableSegment> tableSegments) {
- Map<String, SimpleTableSegment> tableMaps = new HashMap<>(1, 1);
- Collection<SimpleTableSegment> actualTables = new LinkedList<>();
- for (SimpleTableSegment each : tableSegments) {
- if (!tableMaps.containsKey(each.getTableName().getIdentifier().getValue())) {
- tableMaps.put(each.getTableName().getIdentifier().getValue(), each);
- actualTables.add(each);
- }
- }
+ Collection<SimpleTableSegment> actualTables = new LinkedList<>(tableSegments);
+ Set<String> seen = new HashSet<>();
Review comment:
Have you clarified for this question? @MingxingLAI
--
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
[GitHub] [shardingsphere] terrymanu commented on a change in pull request #9967: Issue#9948 refactor TablesContext
Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #9967:
URL: https://github.com/apache/shardingsphere/pull/9967#discussion_r608592261
##########
File path: shardingsphere-infra/shardingsphere-infra-binder/src/main/java/org/apache/shardingsphere/infra/binder/segment/table/TablesContext.java
##########
@@ -46,14 +46,9 @@ public TablesContext(final SimpleTableSegment tableSegment) {
}
public TablesContext(final Collection<SimpleTableSegment> tableSegments) {
- Map<String, SimpleTableSegment> tableMaps = new HashMap<>(1, 1);
- Collection<SimpleTableSegment> actualTables = new LinkedList<>();
- for (SimpleTableSegment each : tableSegments) {
- if (!tableMaps.containsKey(each.getTableName().getIdentifier().getValue())) {
- tableMaps.put(each.getTableName().getIdentifier().getValue(), each);
- actualTables.add(each);
- }
- }
+ Collection<SimpleTableSegment> actualTables = new LinkedList<>(tableSegments);
+ Set<String> seen = new HashSet<>();
Review comment:
What is `seen` mean?
--
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
[GitHub] [shardingsphere] MingxingLAI closed pull request #9967: Issue#9948 refactor TablesContext
Posted by GitBox <gi...@apache.org>.
MingxingLAI closed pull request #9967:
URL: https://github.com/apache/shardingsphere/pull/9967
--
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
[GitHub] [shardingsphere] tristaZero edited a comment on pull request #9967: Issue#9948 refactor TablesContext
Posted by GitBox <gi...@apache.org>.
tristaZero edited a comment on pull request #9967:
URL: https://github.com/apache/shardingsphere/pull/9967#issuecomment-835804846
Hey @MingxingLAI ,
Thanks for your update.
This PR looks plain, just a function name for our consideration. Since we do not prefer an acronym name, could you change it? Is it not knotty, right?
--
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
[GitHub] [shardingsphere] MingxingLAI commented on pull request #9967: Issue#9948 refactor TablesContext
Posted by GitBox <gi...@apache.org>.
MingxingLAI commented on pull request #9967:
URL: https://github.com/apache/shardingsphere/pull/9967#issuecomment-828234518
Any suggestion or question ? @tristaZero @terrymanu
--
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
[GitHub] [shardingsphere] MingxingLAI commented on pull request #9967: Issue#9948 refactor TablesContext
Posted by GitBox <gi...@apache.org>.
MingxingLAI commented on pull request #9967:
URL: https://github.com/apache/shardingsphere/pull/9967#issuecomment-836039944
got it
--
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
[GitHub] [shardingsphere] MingxingLAI commented on pull request #9967: Issue#9948 refactor TablesContext
Posted by GitBox <gi...@apache.org>.
MingxingLAI commented on pull request #9967:
URL: https://github.com/apache/shardingsphere/pull/9967#issuecomment-833259878
> > Any suggestion or question ? @tristaZero @terrymanu
>
> Hi, What is variable name `seen` mean?
Did you notice my previous explanation ？
The `seen` set is used for deduplication, and its function is the same as the original HashMap `tableMaps`
- removeIf will remove an element if it meets the specified criteria
- Set.add will return false if it did not modify the Set, i.e. already contains the value
combining these two, it will remove all elements which has been encountered before.
--
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
[GitHub] [shardingsphere] tristaZero commented on pull request #9967: Issue#9948 refactor TablesContext
Posted by GitBox <gi...@apache.org>.
tristaZero commented on pull request #9967:
URL: https://github.com/apache/shardingsphere/pull/9967#issuecomment-824511765
@terrymanu Please review this one. It is left to stand for a long time.
--
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
[GitHub] [shardingsphere] tristaZero commented on pull request #9967: Issue#9948 refactor TablesContext
Posted by GitBox <gi...@apache.org>.
tristaZero commented on pull request #9967:
URL: https://github.com/apache/shardingsphere/pull/9967#issuecomment-822863845
Hi any progress?
--
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
[GitHub] [shardingsphere] MingxingLAI commented on pull request #9967: Issue#9948 refactor TablesContext
Posted by GitBox <gi...@apache.org>.
MingxingLAI commented on pull request #9967:
URL: https://github.com/apache/shardingsphere/pull/9967#issuecomment-823179093
I have explained the modification of the code, I don’t know what to do next, and I need help from you. @tristaZero
--
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
[GitHub] [shardingsphere] tristaZero commented on pull request #9967: Issue#9948 refactor TablesContext
Posted by GitBox <gi...@apache.org>.
tristaZero commented on pull request #9967:
URL: https://github.com/apache/shardingsphere/pull/9967#issuecomment-835804846
Hey @MingxingLAI
This PR looks plain, just a function name for our consideration. Since we do not prefer an acronym name, could you change it? Is it not knotty, right?
--
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
[GitHub] [shardingsphere] terrymanu commented on pull request #9967: Issue#9948 refactor TablesContext
Posted by GitBox <gi...@apache.org>.
terrymanu commented on pull request #9967:
URL: https://github.com/apache/shardingsphere/pull/9967#issuecomment-830778141
> Any suggestion or question ? @tristaZero @terrymanu
Hi, What is variable name `seen` mean?
--
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