Skip to content

Conversation

@amogh-jahagirdar
Copy link
Contributor

@hililiwei @jackye1995 @flyrain @yyanyy When working on the drop branch implementation, I noticed that the "or" condition for replace branch wasn't placed right in #6638 . It should be within the startsWith("alter table") condition, but currently its outside. The Iceberg DDL will still work, but the check should be more specific to match the alter table condition to prevent any future spark sql syntax change from being bypassed in the isIcebergCommand check.

Just raising this PR separately since it's a bugfix (albeit for an unlikely case)

@github-actions github-actions bot added the spark label Feb 3, 2023
Copy link
Contributor

@jackye1995 jackye1995 left a comment

Choose a reason for hiding this comment

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

oh nice catch, did not realize this at all

@amogh-jahagirdar amogh-jahagirdar force-pushed the drop-branch branch 7 times, most recently from 71e3604 to a7bcb44 Compare February 3, 2023 15:54
@jackye1995
Copy link
Contributor

Thanks for the fixes, let's definitely enable scala checkstyle asap to avoid issues like this.

Also I think there are some other fixes in the tests that Liwei found, could you also fix those?

And the title of the PR should probably change.

@amogh-jahagirdar amogh-jahagirdar changed the title Spark: Fix isIcebergCommand check for replace branch Spark: Fix isIcebergCommand check and style issues from #6638 Feb 3, 2023
@amogh-jahagirdar
Copy link
Contributor Author

amogh-jahagirdar commented Feb 3, 2023

Thanks for the fixes, let's definitely enable scala checkstyle asap to avoid issues like this.

Also I think there are some other fixes in the tests that Liwei found, could you also fix those?

And the title of the PR should probably change.

Done! Agreed I created #6736, to track this to avoid reviewers having to spend time on style issues in the scala codebase. I'm looking into it.

Thanks for the reviews @jackye1995 @hililiwei @yyanyy

@jackye1995
Copy link
Contributor

Thanks, I will merge the change for now, as you are working on fixing Scala checkstyle, we can make any missing changes with that.

@jackye1995 jackye1995 merged commit 7005794 into apache:master Feb 3, 2023
krvikash pushed a commit to krvikash/iceberg that referenced this pull request Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants