Skip to content

Spark: Revert "Spark: Add "Iceberg" prefix to SparkTable name string for SparkUI (#5629)#7273

Merged
jackye1995 merged 1 commit into
apache:masterfrom
amogh-jahagirdar:revert-iceberg-prefix
Apr 3, 2023
Merged

Spark: Revert "Spark: Add "Iceberg" prefix to SparkTable name string for SparkUI (#5629)#7273
jackye1995 merged 1 commit into
apache:masterfrom
amogh-jahagirdar:revert-iceberg-prefix

Conversation

@amogh-jahagirdar

@amogh-jahagirdar amogh-jahagirdar commented Apr 3, 2023

Copy link
Copy Markdown
Contributor

Fixes #7239

This reverts commit fc501f3.

Based on #7239 Spark SHOW CREATE TABLE has a regression in 1.2 because we prefix Iceberg to the SparkTable name. Since name() is a public API, opting to revert the change to maintain the existing behavior. To get the Spark UI we desire, my recommendation is we look at leveraging another API.

@aokolnychyi @RussellSpitzer @sumeetgajjar @szehon-ho @jackye1995 @singhpk234

@RussellSpitzer RussellSpitzer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should probably add a "SHOW CREATE TABLE" test in here

@sririshindra

Copy link
Copy Markdown
Contributor

FYI, @wypoon

@wypoon

wypoon commented Apr 3, 2023

Copy link
Copy Markdown
Contributor

We should probably add a "SHOW CREATE TABLE" test in here

I suggest doing that as a separate change. A revert of a commit should just be a revert, not mixed with other changes.

@amogh-jahagirdar

Copy link
Copy Markdown
Contributor Author

@wypoon I was thinking of adding the tests in a separate commit to preserve the pure revert commit. Then when the change gets merged we make sure we retain both commits instead of squashing

@jackye1995 jackye1995 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thanks for the quick fix!

@jackye1995

Copy link
Copy Markdown
Contributor

Merging, @amogh-jahagirdar could you also help adding a test case against it?

@jackye1995 jackye1995 merged commit 6f7b434 into apache:master Apr 3, 2023
@danielcweeks danielcweeks added this to the Iceberg 1.2.1 milestone Apr 4, 2023
@szehon-ho

Copy link
Copy Markdown
Member

Thanks @amogh-jahagirdar for investigation/ fix !

@RussellSpitzer

Copy link
Copy Markdown
Member

Let's prioritize that SHOW CREATE, test. I really don't want to have a behavior we are guaranteeing that is going untested.

@aokolnychyi

Copy link
Copy Markdown
Contributor

+1 for reverting this.

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.

SHOW CREATE TABLE returns invalid sql after upgrading to 1.2

8 participants