[SPARK-7012][SQL] Add support for NOT NULL modifier for column definitions on DDLParser #8746
Conversation
|
@smola I have created this PR for SPARK-7012 |
|
Can one of the admins verify this patch? |
|
@sabhyankar Great! The implementation looks good. Could you add a test case for it? |
|
@smola Thanks for the review. I will add a test case for it and push another commit today. |
|
@smola I have added a test case for this. Let me know if you see anything else that needs to be changed or added. |
There was a problem hiding this comment.
you can use withTempPath for this test. example
There was a problem hiding this comment.
Thanks @cloud-fan ! This makes it much cleaner. I have made the change in the latest commit.
|
@smola @cloud-fan Thanks again for the review guys. I have made the additional changes that were noted. |
There was a problem hiding this comment.
nit: !xxx.isDefined == xxx.isEmpty
|
LGTM |
|
Thanks again @cloud-fan I have pushed a new commit with the updates! |
|
Hi @smola - Is there anything else that you need me to check for this PR? |
There was a problem hiding this comment.
improve this test case to make sure we really make that column not nullable, i.e. read the table out and check its schema.
There was a problem hiding this comment.
@sabhyankar Do you have time to modify this? I think that once the test is improved with @cloud-fan comment, the change is probably good to merge.
There was a problem hiding this comment.
Hi @smola @cloud-fan - I will take a look at the test case this week and update you guys
|
I am not sure if we should do it now because it is still possible to have nulls in a column defined as |
|
How about we close this PR for now and revisit it later? |
5 participants
Add support for NOT NULL modifier for column definitions in DDLParser