Skip to content

Conversation

@dtenedor
Copy link
Contributor

@dtenedor dtenedor commented Apr 5, 2022

What changes were proposed in this pull request?

Support INSERT INTO commands with user specified column lists with DEFAULT values.

For example:

CREATE TABLE t (x INT DEFAULT 42, y STRING DEFAULT "abc") USING PARQUET;
INSERT INTO t (y) VALUES ("def");
SELECT x, y FROM t;
> 42, "def"

Why are the changes needed?

This PR allows INSERT INTO commands with explicit user specified column lists to benefit from DEFAULT column support, in addition to those without. This expands the functionality provided by this feature.

Does this PR introduce any user-facing change?

Yes, more types of INSERT INTO commands may use DEFAULT values.

How was this patch tested?

This PR adds additional unit test coverage.

@github-actions github-actions bot added the SQL label Apr 5, 2022
@gengliangwang
Copy link
Member

@dtenedor I was focusing on other works today. Will take another look tomorrow.

@dtenedor
Copy link
Contributor Author

dtenedor commented Apr 6, 2022

@dtenedor I was focusing on other works today. Will take another look tomorrow.

That's totally OK! You are finishing up the code sync conductors shift and I am starting the next one today. Thanks again for all your careful review and help.

@dtenedor dtenedor requested a review from gengliangwang April 6, 2022 20:48
@AmplabJenkins
Copy link

Can one of the admins verify this patch?

Copy link
Contributor Author

@dtenedor dtenedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your review! Updated according to excellent suggestions, please take another look.

@dtenedor dtenedor requested a review from gengliangwang April 7, 2022 18:59
@gengliangwang
Copy link
Member

LGTM except a few minor comments.

@dtenedor dtenedor requested a review from gengliangwang April 8, 2022 17:37
Copy link
Contributor Author

@dtenedor dtenedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for your review! This should be ready to merge at your convenience.

Copy link
Member

@gengliangwang gengliangwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the work!

@gengliangwang
Copy link
Member

Merging to master

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants