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

opt: add more tests for user-defined type dependency tracking #85703

Merged
merged 1 commit into from
Aug 25, 2022

Conversation

mgartner
Copy link
Collaborator

@mgartner mgartner commented Aug 6, 2022

This commit updates the expression formatter to format user-defined type
dependencies, which were previously omitted. It also adds some
additional tests to ensure that user-defined type dependency tracking is
working as intended for CREATE VIEW and CREATE FUNCTION.

Release note: None

Release justification: This is a test-only change.

@mgartner mgartner requested a review from a team as a code owner August 6, 2022 21:33
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@mgartner
Copy link
Collaborator Author

mgartner commented Aug 6, 2022

The first commit is from #85700.

@mgartner mgartner force-pushed the improve-typ-deps-tests branch from 2a5d176 to 21fdcd7 Compare August 6, 2022 21:35
├── SELECT fb FROM t.public.foobars
├── columns: fb:1
└── dependencies
└── foobars [columns: fb]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is it expected that we do not track the dependence on the foobar type here? Is this safe because we can determine the transitive dependency of v26 -> foobars -> foobar?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, i think it's safe since it's not directly referenced by the function. I think the bottom line is that everything should still work if workday is dropped/renamed.
That being said, if the UDF references the table workdays as an implicit type, then workday should be a dependency as well. Could you add a test for that as well? Thanks for fixing the formatting!

create-function
├── CREATE FUNCTION f() RETURNS STRING LANGUAGE SQL AS $$SELECT w::STRING FROM t.public.workdays;$$
└── dependencies
└── workdays [columns: w]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Similar question as the one I left in views: is it expected that the workday type is not a dependency here?

Copy link
Contributor

@chengxiong-ruan chengxiong-ruan left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

├── SELECT fb FROM t.public.foobars
├── columns: fb:1
└── dependencies
└── foobars [columns: fb]
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, i think it's safe since it's not directly referenced by the function. I think the bottom line is that everything should still work if workday is dropped/renamed.
That being said, if the UDF references the table workdays as an implicit type, then workday should be a dependency as well. Could you add a test for that as well? Thanks for fixing the formatting!

@mgartner mgartner force-pushed the improve-typ-deps-tests branch from 21fdcd7 to f2dc87c Compare August 8, 2022 14:58
Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 4 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @chengxiong-ruan, @mgartner, and @rytaft)

This commit updates the expression formatter to format user-defined type
dependencies, which were previously omitted. It also adds some
additional tests to ensure that user-defined type dependency tracking is
working as intended for `CREATE VIEW` and `CREATE FUNCTION`.

Release note: None
@mgartner mgartner force-pushed the improve-typ-deps-tests branch from f2dc87c to 9fa4b04 Compare August 22, 2022 15:10
@mgartner
Copy link
Collaborator Author

TFTRs!

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 23, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 23, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 24, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 24, 2022

Build failed:

@mgartner
Copy link
Collaborator Author

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 24, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 24, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 25, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 25, 2022

Build succeeded:

@craig craig bot merged commit 036b50a into cockroachdb:master Aug 25, 2022
@mgartner mgartner deleted the improve-typ-deps-tests branch August 25, 2022 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants