Skip to content
This repository was archived by the owner on May 2, 2023. It is now read-only.

Redshift: support 3 part id #13

Merged
erezsh merged 3 commits into from
Feb 6, 2023
Merged

Redshift: support 3 part id #13

erezsh merged 3 commits into from
Feb 6, 2023

Conversation

@dlawin
Copy link
Contributor

@dlawin dlawin commented Jan 27, 2023

per our slack convo

@dlawin dlawin requested a review from erezsh January 27, 2023 23:48
@dlawin dlawin self-assigned this Jan 27, 2023
@erezsh
Copy link
Contributor

erezsh commented Jan 28, 2023

PR name should mention it's for Redshift.

@dlawin dlawin changed the title override normalize_table_path to support 3 part id Redshift: override normalize_table_path to support 3 part id Jan 28, 2023
@dlawin
Copy link
Contributor Author

dlawin commented Jan 28, 2023

PR name should mention it's for Redshift.

added

@dlawin
Copy link
Contributor Author

dlawin commented Jan 28, 2023

I'll take a deeper look at this on Monday. It looks like 3 part ids are supported for redshift but I could do more testing.

And if we are using 3 part, should use database in queries

@dlawin dlawin changed the title Redshift: override normalize_table_path to support 3 part id Redshift: support 3 part id Jan 30, 2023
@dlawin dlawin closed this Feb 1, 2023
@dlawin dlawin reopened this Feb 1, 2023
@dlawin
Copy link
Contributor Author

dlawin commented Feb 2, 2023

Do I need to extend the URI params?

dlawin
dlawin commented Feb 2, 2023
View reviewed changes
@dlawin
Copy link
Contributor Author

dlawin commented Feb 5, 2023

@erezsh Are you good with this change? Any tests I can add to ease concerns of a regression?

@erezsh
Copy link
Contributor

erezsh commented Feb 5, 2023

The tests didn't run on redshift yet. We need to set it back up.

@dlawin
Copy link
Contributor Author

dlawin commented Feb 5, 2023

The tests didn't run on redshift yet. We need to set it back up.

Oh is the URI null or something? Semi-related it'd be nice to have admin on this repo, even to just look at settings and stuff

@erezsh erezsh merged commit f415f5a into master Feb 6, 2023
@erezsh
Copy link
Contributor

erezsh commented Feb 6, 2023

LGTM

@erezsh
Copy link
Contributor

erezsh commented Feb 6, 2023

As for admin privs, that's not up to me. But if we add everyone I'm worried it might turn into a "too many cooks" situation.

This was referenced Feb 6, 2023
Merged
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@erezsh erezsh erezsh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@dlawin dlawin

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dlawin @erezsh