Skip to content

Conversation

@gengliangwang
Copy link
Member

What changes were proposed in this pull request?

In the previous work of csv/json migration, CSVFileFormat/JsonFileFormat is removed in the table provider whitelist of AlterTableAddColumnsCommand.verifyAlterTableAddColumn:
#24005
#24058

This is regression. If a table is created with Provider org.apache.spark.sql.execution.datasources.csv.CSVFileFormat or org.apache.spark.sql.execution.datasources.json.JsonFileFormat, Spark should allow the "alter table add column" operation.

How was this patch tested?

Unit test

@gengliangwang
Copy link
Member Author

@dongjoon-hyun

@emanuelebardelli
Copy link

hi @gengliangwang

i just submit this PR (#24780) that actually refactors AlterTableAddColumnsCommand.verifyAlterTableAddColumn quite a bit, then i noticed your PR...

Maybe it's worth if i merge your changes into my PR? Any other idea?

@SparkQA
Copy link

SparkQA commented Jun 3, 2019

Test build #106104 has finished for PR 24776 at commit ff3bb1f.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@gengliangwang
Copy link
Member Author

gengliangwang commented Jun 3, 2019

@emanuelebardelli I think we can merge this one first, and then you can update your refactoring PR.
ping @dongjoon-hyun @gatorsmile @HyukjinKwon

// come in here.
case _: JsonDataSourceV2 | _: CSVDataSourceV2 | _: ParquetFileFormat | _: OrcDataSourceV2 =>
case _: CSVFileFormat | _: JsonFileFormat | _: ParquetFileFormat =>
case _: JsonDataSourceV2 | _: CSVDataSourceV2 | _: OrcDataSourceV2 =>
Copy link
Member

Choose a reason for hiding this comment

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

These V2 data sources also support ADD COLUMNs? Do we have the test cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, V2 doesn't support ADD COLUMN. If it requires catalog support, Spark will fall back V2 to V1.

Currently the result of DataSource.lookupDataSource for "csv"/"json"/"orc" will always be CSVDataSourceV2/JsonDataSourceV2/OrcDataSourceV2. So we need to match them here.

@gatorsmile
Copy link
Member

LGTM

Thanks! Merged to master.

@gatorsmile gatorsmile closed this in d1937c1 Jun 4, 2019
Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

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

LGTM too

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.

6 participants