[SPARK-29853][SQL]lpad returning empty instead of NULL for empty pad … #26478
Conversation
| if (len > 0 && pad.numBytes() == 0) { | ||
| // no padding at all, return the null | ||
| return null; | ||
| } else if (spaces <= 0) { |
There was a problem hiding this comment.
I don't think it's correct to fix it here since the issue is specific to lpad expression. Can you check lpad in other DBMSes? I think we shouldn't just blindly follow Hive
There was a problem hiding this comment.
@HyukjinKwon, fine i will check this point.
There was a problem hiding this comment.
@HyukjinKwon, I checked in other DBMSes , please check below DBMSES output for lpad and rpad :
- Mysql : rpad ('hi', 5, '') => NULL and lpad ('hi', 5, '') => NULL
- Hive : rpad ('hi', 5, '') => NULL and lpad ('hi', 5, '') => NULL
- Postgresql : rpad ('hi', 5, '') => hi and lpad ('hi', 5, '') => hi
There was a problem hiding this comment.
I think it's fine not to fix for now.
There was a problem hiding this comment.
Current implementation is at least reasonable. If we should do, we could add a configuration to control the behaviour but to me it doesn't look worthy.
There was a problem hiding this comment.
But as per rpad/lpad definition 'In case of empty pad string, the return value is null.' If we are not going to handle in 3.0 we can move to handle this in next version
|
Can one of the admins verify this patch? |
|
@HyukjinKwon , please check and let me know whether we need to fix this issue ? |
5 participants
What changes were proposed in this pull request?
lpad returning NULL instead of empty for empty pad value.
Why are the changes needed?
Need to add check-point if padding string is empty and require length is greater then zero.
Does this PR introduce any user-facing change?
NO
How was this patch tested?
Old unit tests correct as per this jira.