Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

optimize find column index #1394

Closed
wants to merge 2 commits into from

Conversation

Okapist
Copy link
Contributor

@Okapist Okapist commented Jul 8, 2023

Summary

ClickhouseResultSet look for column index on each row. It use expensive equalsIgnoreCase method.
I added index cache for it.

@CLAassistant
Copy link

CLAassistant commented Jul 8, 2023

CLA assistant check
All committers have signed the CLA.

@Okapist
Copy link
Contributor Author

Okapist commented Jul 8, 2023

Collections.synchronizedMap(new HashMap<>()) may be replaced with ConcurrentHashMap if not need to support old jdk versions.

@zhicwu
Copy link
Contributor

zhicwu commented Jul 12, 2023

Thank you @Okapist for the pull request and sorry for the late reply.

Can we create the HashMap only when needed(e.g. first time calling findColumnIndex)? Besides, when instantiate the cache, can we specify the load factor since we know the maximum number of columns?

Collections.synchronizedMap(new HashMap<>()) may be replaced with ConcurrentHashMap if not need to support old jdk versions.

Perhaps AtomicReference with lazy load will be slightly faster. Having said that, the current implementation is not thread-safe, so I'm good even we use HashMap :)

@Okapist
Copy link
Contributor Author

Okapist commented Jul 13, 2023

Can we create the HashMap only when needed(e.g. first time calling findColumnIndex)? Besides, when instantiate the cache, can we specify the load factor since we know the maximum number of columns?

Good idea. Pull request updated.

@zhicwu
Copy link
Contributor

zhicwu commented Jul 14, 2023

Thanks @Okapist. Will merge this one after fixing build failure.

@chernser
Copy link
Contributor

Newer PR merged #1568

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants