-
Notifications
You must be signed in to change notification settings - Fork 288
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
[#3136] fix(spark-connector): support replace column with same column name for spark connector #3461
Conversation
@jerryshao @mchades @qqqttt123 please help to review this PR when you are free, thanks |
...ration-test/src/test/java/com/datastrato/gravitino/integration/test/spark/SparkCommonIT.java
Outdated
Show resolved
Hide resolved
...ration-test/src/test/java/com/datastrato/gravitino/integration/test/spark/SparkCommonIT.java
Outdated
Show resolved
Hide resolved
63b4b47
to
133c7b3
Compare
@@ -171,40 +171,7 @@ private void doUpdateColumnType( | |||
icebergUpdateSchema.updateColumn(fieldName, (PrimitiveType) type); | |||
} | |||
|
|||
private ColumnPosition getAddColumnPosition(StructType parent, ColumnPosition columnPosition) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iceberg lib will handle this internally
@mchades @jerryshao please help to review when you are free, thanks. |
@@ -143,7 +142,8 @@ private void doMoveColumn( | |||
} else if (columnPosition instanceof TableChange.First) { | |||
icebergUpdateSchema.moveFirst(DOT.join(fieldName)); | |||
} else { | |||
throw new NotSupportedException( | |||
Preconditions.checkArgument( | |||
columnPosition instanceof TableChange.Default, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change mean that if the user does not specify the column position, nothing will happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, Iceberg will treat it as last
position.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But here is the end of the block, I don't see any action after the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iceberg will add the column to the last
position, so no need to move the column
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that this method is called in two places, one is doAddColumn
and the other is doUpdateColumnPosition
. If it's called by doUpdateColumnPosition
, no changes will be made either. Are you sure this meets expectations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this seems bugy, let me think about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mchades , fix it and add test, please help to review again
@@ -156,7 +160,9 @@ private void doUpdateColumnType( | |||
icebergUpdateSchema.updateColumn(fieldName, (PrimitiveType) type); | |||
} | |||
|
|||
private ColumnPosition getAddColumnPosition(StructType parent, ColumnPosition columnPosition) { | |||
// Iceberg doesn't support LAST |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iceberg doesn't support LAST, so what do you do in the method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method transform Last
Position to After
, or first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update the comment
@mchades all comments are addressed, please review again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What changes were proposed in this pull request?
To support replace column:
same column name
in valid column change logic in hive catalogWhy are the changes needed?
Fix: #3136
Does this PR introduce any user-facing change?
no
How was this patch tested?
add IT