[SPARK-22452][SQL]Add getInt, getLong, getBoolean to DataSourceV2Options#19902
[SPARK-22452][SQL]Add getInt, getLong, getBoolean to DataSourceV2Options#19902skambha wants to merge 5 commits intoapache:masterfrom
Conversation
skambha
commented
Dec 5, 2017
skambha
commented
- Implemented methods getInt, getLong, getBoolean for DataSourceV2Options
- Added new unit tests to exercise these methods
|
ok to test |
|
Test build #84514 has finished for PR 19902 at commit
|
| return Optional.ofNullable(keyLowerCasedMap.get(toLowerCase(key))); | ||
| } | ||
|
|
||
| public Optional<Boolean> getBoolean(String key) { |
There was a problem hiding this comment.
to avoid the expensive boxing, how about public boolean getBoolean(String key, boolean defaultValue)? This is also consistent with SparkConf.getXXX
There was a problem hiding this comment.
good idea.
-
So I have implemented the getBoolean, getInt, and getLong using the defaultValue and added unit tests for it.
-
For now, I have left the other methods as well that return Optional<> keeping in line with the getString as it may still have some value.
Please take a look.
If we don't want (2), I can remove the version of the getBoolean, getLong, getInt that doesn't take the defaultValue.
Thanks for your comments.
There was a problem hiding this comment.
Per @gatorsmile , I have gone and removed the version of the methods that doesn't take the default value.
| Integer.parseInt(keyLowerCasedMap.get(lcaseKey)) : null); | ||
| } | ||
|
|
||
| public Optional<Long> getLong(String key) { |
There was a problem hiding this comment.
Can you also remove the three APIs without the default values?
There was a problem hiding this comment.
Sure. Let me post another commit. Thanks.
| public Optional<String> get(String key) { | ||
| return Optional.ofNullable(keyLowerCasedMap.get(toLowerCase(key))); | ||
| } | ||
|
|
There was a problem hiding this comment.
/** Get a parameter as a boolean, falling back to a default if not set */
There was a problem hiding this comment.
Thanks. Is it ok if I modify the comment for the method a bit to this:
/**
* Returns the boolean value to which the specified key is mapped,
* or defaultValue if there is no mapping for the key.
* The key match is case-insensitive
*/
| Boolean.parseBoolean(keyLowerCasedMap.get(lcaseKey)) : defaultValue; | ||
| } | ||
|
|
||
| public int getInt(String key, int defaultValue) { |
There was a problem hiding this comment.
/** Get a parameter as an integer, falling back to a default if not set */
| Integer.parseInt(keyLowerCasedMap.get(lcaseKey)) : defaultValue; | ||
| } | ||
|
|
||
| public long getLong(String key, long defaultValue) { |
There was a problem hiding this comment.
/** Get a parameter as a long, falling back to a default if not set */
|
LGTM except three comments. |
|
great! Thanks @gatorsmile for your comments. I have updated with code comments, please take a look. Thanks. |
|
Test build #84578 has finished for PR 19902 at commit
|
|
Test build #84579 has finished for PR 19902 at commit
|
|
Test build #84582 has finished for PR 19902 at commit
|
|
thanks, merging to master! @skambha would you like to open a new PR to add the rest of |
|
great! Thanks @gatorsmile , @cloud-fan. Yes. I would be happy to open a new PR to add the rest of them. If I compare with the getXXX (for datatype) in SparkConf, it looks like the only missing one is to add getDouble. Do we want to add only that or any of the other datatypes or any other getXXX? Let me know and I would be happy to open a new PR to address it. Thank you! |
|
ah i see, yea looks like |
|
cool! I will add that and submit a new PR. |
|
Opened a new PR to add the getDouble method. #19921 |
…ions - Implemented methods getInt, getLong, getBoolean for DataSourceV2Options - Added new unit tests to exercise these methods Author: Sunitha Kambhampati <skambha@us.ibm.com> Closes apache#19902 from skambha/spark22452.
4 participants
