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

Test and Update MInVersionsFixedIn for issue of ALTER PARTITIONED TABLE ADD PRIMARY KEY #2047

Merged
merged 11 commits into from
Dec 11, 2024

Conversation

makalaaneesh
Copy link
Collaborator

@makalaaneesh makalaaneesh commented Dec 9, 2024

Describe the changes in this pull request

  • Added unit test
  • Updated MinimumVersionsFixedIn

Describe if there are any user-facing changes

How was this pull request tested?

Does your PR have changes that can cause upgrade issues?

Component Breaking changes?
MetaDB Yes/No
Name registry json Yes/No
Data File Descriptor Json Yes/No
Export Snapshot Status Json Yes/No
Import Data State Yes/No
Export Status Json Yes/No
Data .sql files of tables Yes/No
Export and import data queue Yes/No
Schema Dump Yes/No
AssessmentDB Yes/No
Sizing DB Yes/No
Migration Assessment Report Json Yes/No
Callhome Json Yes/No
YugabyteD Tables Yes/No
TargetDB Metadata Tables Yes/No

@makalaaneesh makalaaneesh changed the title Test for issue of ALTER PARTITIONED TABLE ADD PRIMARY KEY Test and Update MInVersionsFixedIn for issue of ALTER PARTITIONED TABLE ADD PRIMARY KEY Dec 11, 2024
@makalaaneesh makalaaneesh marked this pull request as ready for review December 11, 2024 10:27
Copy link
Contributor

@priyanshi-yb priyanshi-yb left a comment

Choose a reason for hiding this comment

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

LGTM

ybVersion := os.Getenv("YB_VERSION")
if ybVersion == "" {
panic("YB_VERSION env variable is not set. Set YB_VERSIONS=2024.1.3.0-b105 for example")
}

ybVersionWithoutBuild := strings.Split(ybVersion, "-")[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: so our --target-db-version can only accept the version without build number only?
Just asking this if we are mentioning this in flag's help msg ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, right now it does accept build number as well, but the problem is that 2024.2.0.0-b145 is considered to be > 2024.2.0.0 (which is defined in the MinVersionsFixedIn).

I'm planning to raise another PR preventing users from passing the build number in the input to prevent these kinds of issues.

) PARTITION BY RANGE (order_date);
ALTER TABLE orders2 ADD PRIMARY KEY (order_id,order_date)`)

assertErrorCorrectlyThrownForIssueForYBVersion(t, err, "changing primary key of a partitioned table is not yet implemented", alterTableAddPKOnPartitionIssue, testYbVersion)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no need to pass testYbVersion as its global var

@makalaaneesh makalaaneesh merged commit 7990563 into main Dec 11, 2024
34 checks passed
@makalaaneesh makalaaneesh deleted the aneesh/partitioned-tables-pk-issue branch December 11, 2024 16:07
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