Skip to content

Case Insensitive Map for getting Column Index#1572

Merged
enisoc merged 6 commits into
masterfrom
caseInsensitiveMap
Apr 4, 2016
Merged

Case Insensitive Map for getting Column Index#1572
enisoc merged 6 commits into
masterfrom
caseInsensitiveMap

Conversation

@harshit-gangal
Copy link
Copy Markdown
Member

This is needed so that application does not have to take care of exact case of the column.
When a V2 API is called, the column can be in Uppercase or in Mixed Case.
When a V3 API is called, the column are in Lowercase.

It is hard for the application to track when to call with Exact Matching case and when to call in lowercase.

Use of CaseInsensitiveMap will help application to call in case insensitive manner.


This change is Reviewable

@googlebot
Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@sougou
Copy link
Copy Markdown
Contributor

sougou commented Mar 16, 2016

The v2 behavior is inconsistent. It should also return lower-cased column names just like v3.
Nevertheless, it's a good idea to use case-insensitive comparisons on the client side.

@enisoc
Copy link
Copy Markdown
Contributor

enisoc commented Mar 16, 2016

Thanks for turning this into a PR. I see what you mean now. I found this in the JDBC ResultSet spec:

Column names used as input to getter methods are case insensitive. When a getter method is called with a column name and several columns have the same name, the value of the first matching column will be returned.

So I definitely agree we should do this. Do you foresee any problems if we implement it with String.toLowerCase() and ImmutableMap? It would be nice to be able to still keep the map immutable. It seems like CaseInsensitiveMap might be a little smarter about being locale-independent. Have you guys seen problems with locales when using String.toLowerCase()? For example, I noticed you or your teammate implemented your own case-insensitive string search in another PR rather than using toLowerCase() and indexOf(), and I haven't understood why yet.

We should also add a test case to verify this behavior, and add to the javadocs for FieldMap and the Row getters to notify users that columnLabel is case-insensitive. I can do that as a separate PR if you don't have time.

@enisoc
Copy link
Copy Markdown
Contributor

enisoc commented Mar 16, 2016

As a side note regarding the quote from the spec above, we currently will return the last matching column if multiple columns have the same name. We may consider changing that to match ResultSet as well.

@harshit-gangal
Copy link
Copy Markdown
Member Author

I have changed it according to ResultSet specs.
@enisoc Please go ahead and make the changes.

Anthony Yeh added 2 commits March 16, 2016 15:48
@enisoc
Copy link
Copy Markdown
Contributor

enisoc commented Mar 16, 2016

@harshit-gangal Thanks. I added the docs and test as a new commit on your branch. I decided not to use ImmutableMap after all, because you can't use the map to check for duplicates while building the map.

This should be ready to merge once your company's CLA goes through.

@enisoc enisoc self-assigned this Mar 17, 2016
@harshit-gangal
Copy link
Copy Markdown
Member Author

CLA is signed


Comments from the review on Reviewable.io

@googlebot
Copy link
Copy Markdown

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Apr 3, 2016
@harshit-gangal
Copy link
Copy Markdown
Member Author

Approve the pull request here https://pullapprove.com/youtube/vitess/pull-request/1572/

@enisoc
Copy link
Copy Markdown
Contributor

enisoc commented Apr 4, 2016

:lgtm:


Reviewed 1 of 2 files at r1, 4 of 4 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

Approved with PullApprove

@enisoc enisoc merged commit 6b7efe5 into master Apr 4, 2016
@enisoc enisoc deleted the caseInsensitiveMap branch April 4, 2016 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants