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

IMPORT INTO with default expressions and computed columns #8764

Merged
merged 3 commits into from
Nov 4, 2020

Conversation

lnhsingh
Copy link
Contributor

Closes #8585.
Closes #8291.
Closes #8105.
Closes #8289.
Closes #8106.
Closes #7863.
Closes #8419.

Closes #8585.
Closes #8291.
Closes #8105.
Closes #8289.
Closes #8106.
Closes #7863.
Closes #8419.
@lnhsingh lnhsingh requested review from pbardea and mwang1026 October 27, 2020 14:56
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@lnhsingh
Copy link
Contributor Author

@pbardea - friendly ping to PTAL early next week :) I'd like to address reviews and merge this PR this upcoming week!

Copy link
Contributor

@pbardea pbardea left a comment

Choose a reason for hiding this comment

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

Sorry for the delay here!

This generally LGTM, with a small comment about the callout and a nit. Thanks!

- `transaction_timestamp()`

- `random()`
- `gen_random_uuid`
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: parens here to match the rest?

- `unique_rowid()`

{{site.data.alerts.callout_info}}
Non-targeted columns with constant default expressions are not required to be nullable.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that we define what "targeted" columns are in the docs? Do we have a way of referring to the columns that are specified in an IMPORT INTO command?

Thinking about this more, do we usually have callouts for things that are not required? I think this behaviour would be the "expected" case, rather than an exceptional one.

Copy link
Contributor Author

@lnhsingh lnhsingh left a comment

Choose a reason for hiding this comment

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

TFTR!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @mwang1026 and @pbardea)


v20.2/import-into.md, line 98 at r2 (raw file):

Previously, pbardea (Paul Bardea) wrote…

nit: parens here to match the rest?

Done.


v20.2/import-into.md, line 102 at r2 (raw file):

Previously, pbardea (Paul Bardea) wrote…

I'm not sure that we define what "targeted" columns are in the docs? Do we have a way of referring to the columns that are specified in an IMPORT INTO command?

Thinking about this more, do we usually have callouts for things that are not required? I think this behaviour would be the "expected" case, rather than an exceptional one.

Yeah, that's a good point. I don't think we need the callout. Removed.

@lnhsingh lnhsingh requested a review from taroface November 2, 2020 20:51
Copy link
Contributor

@taroface taroface left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mwang1026, @pbardea, and @taroface)

@lnhsingh lnhsingh merged commit 5fd70bc into master Nov 4, 2020
@lnhsingh lnhsingh deleted the import-into-default-exp branch November 4, 2020 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment