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/08/23 19:55:12 UTC

[GitHub] [shardingsphere] rfscouto opened a new pull request #11964: Make findColumn case insensitive

rfscouto opened a new pull request #11964:
URL: https://github.com/apache/shardingsphere/pull/11964


   Fixes #5432 
   
   Changes proposed in this pull request:
   - Make a similar implementation to PgResultSet#findColumn
   - This implementation can, and should, be improved as necessity arises.
   - Implemented minor test, can improve further upon recommendation
   


-- 
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: notifications-unsubscribe@shardingsphere.apache.org

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

[GitHub] [shardingsphere] strongduanmu commented on pull request #11964: Make findColumn case insensitive

Posted by GitBox <gi...@apache.org>.

strongduanmu commented on pull request #11964:
URL: https://github.com/apache/shardingsphere/pull/11964#issuecomment-904321080


   Thank you for your pr @rfscouto, I have left some comments, welcome to reply. BTW, Please fix the CI exception first.


-- 
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: notifications-unsubscribe@shardingsphere.apache.org

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

[GitHub] [shardingsphere] tristaZero commented on pull request #11964: Make findColumn case insensitive

Posted by GitBox <gi...@apache.org>.

tristaZero commented on pull request #11964:
URL: https://github.com/apache/shardingsphere/pull/11964#issuecomment-904315000


   @rfscouto Thanks for your work on this legacy issue. @sandynz @strongduanmu could you review this PR? It's about #5432.


-- 
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: notifications-unsubscribe@shardingsphere.apache.org

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

[GitHub] [shardingsphere] strongduanmu commented on a change in pull request #11964: Make findColumn case insensitive

Posted by GitBox <gi...@apache.org>.

strongduanmu commented on a change in pull request #11964:
URL: https://github.com/apache/shardingsphere/pull/11964#discussion_r694488441



##########
File path: shardingsphere-jdbc/shardingsphere-jdbc-core/src/main/java/org/apache/shardingsphere/driver/jdbc/core/resultset/DatabaseMetaDataResultSet.java
##########
@@ -30,15 +30,7 @@
 import java.sql.SQLException;
 import java.sql.Time;
 import java.sql.Timestamp;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.Iterator;
-import java.util.LinkedList;
-import java.util.List;
-import java.util.Map;
-import java.util.Optional;
+import java.util.*;

Review comment:
       @rfscouto Please do not use `*` to import. 

##########
File path: shardingsphere-jdbc/shardingsphere-jdbc-core/src/main/java/org/apache/shardingsphere/driver/jdbc/core/resultset/DatabaseMetaDataResultSet.java
##########
@@ -358,10 +350,27 @@ public Object getObject(final String columnLabel) throws SQLException {
     @Override
     public int findColumn(final String columnLabel) throws SQLException {
         checkClosed();
-        if (!columnLabelIndexMap.containsKey(columnLabel)) {
-            throw new SQLException(String.format("Can not find columnLabel %s", columnLabel));
+
+        Integer columnIndex = columnLabelIndexMap.get(columnLabel);

Review comment:
       @rfscouto The implementation of this logic is a bit complicated. Can we consider using `Map<String, Integer> columnLabelIndexMap  = new TreeMap<>(String.CASE_INSENSITIVE_ORDER)` to solve this exception?




-- 
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: notifications-unsubscribe@shardingsphere.apache.org

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

[GitHub] [shardingsphere] strongduanmu merged pull request #11964: Make findColumn case insensitive

Posted by GitBox <gi...@apache.org>.

strongduanmu merged pull request #11964:
URL: https://github.com/apache/shardingsphere/pull/11964


   


-- 
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: notifications-unsubscribe@shardingsphere.apache.org

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

[GitHub] [shardingsphere] rfscouto commented on a change in pull request #11964: Make findColumn case insensitive

Posted by GitBox <gi...@apache.org>.

rfscouto commented on a change in pull request #11964:
URL: https://github.com/apache/shardingsphere/pull/11964#discussion_r694574013



##########
File path: shardingsphere-jdbc/shardingsphere-jdbc-core/src/main/java/org/apache/shardingsphere/driver/jdbc/core/resultset/DatabaseMetaDataResultSet.java
##########
@@ -358,10 +350,27 @@ public Object getObject(final String columnLabel) throws SQLException {
     @Override
     public int findColumn(final String columnLabel) throws SQLException {
         checkClosed();
-        if (!columnLabelIndexMap.containsKey(columnLabel)) {
-            throw new SQLException(String.format("Can not find columnLabel %s", columnLabel));
+
+        Integer columnIndex = columnLabelIndexMap.get(columnLabel);

Review comment:
       That is completely fine by me. It is exactly what I had initial though about, but then read https://github.com/apache/shardingsphere/issues/5432 and thought we wanted to be aligned with the implementation of one of the big ones (and I made it aligned with PostgreSQL).
   Converted to TreeMap.




-- 
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: notifications-unsubscribe@shardingsphere.apache.org

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

[GitHub] [shardingsphere] rfscouto commented on a change in pull request #11964: Make findColumn case insensitive

Posted by GitBox <gi...@apache.org>.

rfscouto commented on a change in pull request #11964:
URL: https://github.com/apache/shardingsphere/pull/11964#discussion_r694574252



##########
File path: shardingsphere-jdbc/shardingsphere-jdbc-core/src/main/java/org/apache/shardingsphere/driver/jdbc/core/resultset/DatabaseMetaDataResultSet.java
##########
@@ -30,15 +30,7 @@
 import java.sql.SQLException;
 import java.sql.Time;
 import java.sql.Timestamp;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.Iterator;
-import java.util.LinkedList;
-import java.util.List;
-import java.util.Map;
-import java.util.Optional;
+import java.util.*;

Review comment:
       Thanks! The validation was also failing for this :)




-- 
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: notifications-unsubscribe@shardingsphere.apache.org

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