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

[Redshift] Increase String Precision - Part 2 #955

Merged
merged 24 commits into from
Oct 16, 2024

Conversation

Tang8330
Copy link
Contributor

@Tang8330 Tang8330 commented Oct 8, 2024

Part one - #953

Background

This is the second PR to enable automatic string precision for Redshift.

Scope

  • This PR adds the ability to update both staging and temporary table's string precision (if necessary).
  • Adds the dialect to increase string precision

@Tang8330 Tang8330 changed the title [WIP] - Redshift string precision - Part two [Redshift] Increase String Precision - Part 2 Oct 8, 2024
@Tang8330 Tang8330 marked this pull request as ready for review October 8, 2024 15:59
@Tang8330 Tang8330 requested a review from a team as a code owner October 8, 2024 15:59
@@ -126,6 +127,10 @@ func (c *Columns) UpsertColumn(colName string, arg UpsertColumnArg) {
col.backfilled = *arg.Backfilled
}

if arg.StringPrecision != nil {
col.KindDetails.OptionalStringPrecision = arg.StringPrecision
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this has access to the existing column metadata, is this a better place to check that the new precision is greater than the previous?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already have guardrail in how NewLength is being set, I can add something here as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

My main concern was addressed on the other comment, but yeah it might be nice to add a backup check here too, just in case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I don't see why not. I'll add it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm gonna open up another PR to add error to the signature for UpsertColumn so there's less diff

@Tang8330 Tang8330 merged commit 3c03abf into master Oct 16, 2024
3 checks passed
@Tang8330 Tang8330 deleted the redshift-string-precision branch October 16, 2024 03:16
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.

2 participants