Skip to content

[#4412] feat(api): add validate check for default position when updating column position#4451

Merged
FANNG1 merged 12 commits intoapache:mainfrom
LiuQhahah:#4412-add-validate-check-for-ColumnPosition-when-updating-column-positions
Aug 13, 2024
Merged

[#4412] feat(api): add validate check for default position when updating column position#4451
FANNG1 merged 12 commits intoapache:mainfrom
LiuQhahah:#4412-add-validate-check-for-ColumnPosition-when-updating-column-positions

Conversation

@LiuQhahah
Copy link
Contributor

@LiuQhahah LiuQhahah commented Aug 8, 2024

What changes were proposed in this pull request?

add validate check for ColumnPosition when updating column positions,
If the position is default, will throw exception.

Why are the changes needed?

Fix: #4412

Does this PR introduce any user-facing change?

No

How was this patch tested?

the UT has been added.

@FANNG1
Copy link
Contributor

FANNG1 commented Aug 8, 2024

@LiuQhahah , please fix the ci and test failures like CatalogIcebergBaseIT#testUpdateIcebergColumnDefaultPosition

@LiuQhahah
Copy link
Contributor Author

I have fixed the testUpdateIcebergColumnDefaultPosition UT and formated the code.

…n't allow default position in UpdateColumnPosition
@jerryshao
Copy link
Contributor

Looks like there's some style issue needs to be fixed, @LiuQhahah would you please fix it.

…or-ColumnPosition-when-updating-column-positions' into apache#4412-add-validate-check-for-ColumnPosition-when-updating-column-positions
@FANNG1
Copy link
Contributor

FANNG1 commented Aug 13, 2024

There're some residual logic to process DefaultColumnPosition in IcebergTableOpsHelper.java, do you like to remove it in this PR?

  private void doUpdateColumnPosition(
      UpdateSchema icebergUpdateSchema,
      UpdateColumnPosition updateColumnPosition,
      Schema icebergTableSchema) {
    StructType tableSchema = icebergTableSchema.asStruct();
    // not need any more
    ColumnPosition columnPosition =
        getColumnPositionForIceberg(tableSchema, updateColumnPosition.getPosition());
    doMoveColumn(icebergUpdateSchema, updateColumnPosition.fieldName(), columnPosition);
  }

…n due to supporting update the column position to default position isn't allowed.
…or-ColumnPosition-when-updating-column-positions' into apache#4412-add-validate-check-for-ColumnPosition-when-updating-column-positions
@FANNG1 FANNG1 changed the title #4412 add validate check for ColumnPosition when updating column posi… [#4412] feat(api): add validate check for ColumnPosition when updating column posi… Aug 13, 2024
@FANNG1 FANNG1 changed the title [#4412] feat(api): add validate check for ColumnPosition when updating column posi… [#4412] feat(api): add validate check for ColumnPosition when updating column position Aug 13, 2024
@FANNG1 FANNG1 changed the title [#4412] feat(api): add validate check for ColumnPosition when updating column position [#4412] feat(api): add validate check for default position when updating column position Aug 13, 2024
@FANNG1 FANNG1 merged commit a8de245 into apache:main Aug 13, 2024
@FANNG1
Copy link
Contributor

FANNG1 commented Aug 13, 2024

@LiuQhahah , thanks for your contribution, and hope you enjoy the journey.

@LiuQhahah LiuQhahah deleted the #4412-add-validate-check-for-ColumnPosition-when-updating-column-positions branch August 13, 2024 14:31
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.

[Improvement] add validate check for ColumnPosition when updating column positions.

3 participants