Skip to content

[MINOR] Support column type evolution for Hive - #7129

Open
fsilent wants to merge 6 commits into
apache:masterfrom
fsilent:master
Open

[MINOR] Support column type evolution for Hive#7129
fsilent wants to merge 6 commits into
apache:masterfrom
fsilent:master

Conversation

@fsilent

@fsilentfsilent commented Nov 3, 2022

Copy link
Copy Markdown
Contributor

Change Logs

#6989 cannot support column type evolution.

class that implements SelfDescribingInputFormatInterface,
hive will try to do colum type evolution by itself

Impact

support type change

Risk level (write none, low medium or high below)

low.

Documentation Update

N/A

Contributor's checklist

  • Read through contributor's guide
  • Change Logs and Impact were stated clearly
  • Adequate tests were added if applicable
  • CI passed

@fsilent

Copy link
Copy Markdown
ContributorAuthor

@xiarixiaoyao please help to review.

@xiarixiaoyao
xiarixiaoyao self-requested a review November 3, 2022 12:00
@xiarixiaoyaoxiarixiaoyao self-assigned this Nov 3, 2022
@xiarixiaoyaoxiarixiaoyao changed the title [MINOR] Support schema evolution for Hive[MINOR] Support column type evolution for HiveNov 3, 2022
@fsilentfsilent closed this Nov 3, 2022
@fsilentfsilent reopened this Nov 3, 2022
</dependency>

<!-- Hive -->
<dependency>

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.

why we need hive dependency here?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

changed. now dont need to add hive dependency

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.

This is still showing up. Can you remove this dependency as the change is in hudi-hadoop-mr package and hive-exec is already defined as dependency in hudi-hadoop-mr /pom.xml

@UseFileSplitsFromInputFormat
public class HoodieParquetInputFormat extends HoodieParquetInputFormatBase {
public class HoodieParquetInputFormat extends HoodieParquetInputFormatBase implements
SelfDescribingInputFormatInterface {

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.

it will be better to implements SelfDescribingInputFormatInterface by HoodieParquetInputFormatBase
let's move this change to HoodieParquetInputFormatBase

public abstract class HoodieParquetInputFormatBase extends MapredParquetInputFormat implements Configurable, SelfDescribingInputFormatInterface

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ok. has changed

@leesfleesf 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.

LGTM, Please the CI failure and fix

@nsivabalannsivabalan added priority:high Significant impact; potential bugs engine:hive Hive integration reader-core labels Nov 7, 2022
@fsilent

Copy link
Copy Markdown
ContributorAuthor

Because of SelfDescribingInputFormatInterface not exist in hive 1.x, so i have to add dependency in hudi-spark-common. Otherwise, the SelfDescribingInputFormatInterface class cannot be found if Hive 1.x is used during hudi-spark-common compilation.

@xiarixiaoyao

Copy link
Copy Markdown
Contributor

@leesf@xushiyan could you pls help review again thanks
an additional question
What is the current community attitude towards hive1.x
hive1.x cannot support type change This is hive's own limitation, not hudi

@xiarixiaoyao

Copy link
Copy Markdown
Contributor

@hudi-bot run azure

@fsilent

Copy link
Copy Markdown
ContributorAuthor

@hudi-bot run azure

@leesf

leesf commented Nov 9, 2022

Copy link
Copy Markdown
Contributor

@leesf@xushiyan could you pls help review again thanks an additional question What is the current community attitude towards hive1.x hive1.x cannot support type change This is hive's own limitation, not hudi

I do not think we need to support so old hive version for this feature.

@xushiyanxushiyan self-assigned this Nov 9, 2022
@xiarixiaoyao

Copy link
Copy Markdown
Contributor

@hudi-bot run azure

@hudi-bot

Copy link
Copy Markdown
Collaborator

CI report:

Bot commands@hudi-bot supports the following commands:
  • @hudi-bot run azure re-run the last Azure build

@pratyakshsharma

pratyakshsharma commented Nov 16, 2022

Copy link
Copy Markdown
Contributor

LGTM as well, though I am still curious to understand in detail how does this interface SelfDescribingInputFormatInterface help in achieving column type evolution. I tried searching for docs, but could not find anything. If you can point me to relevant docs, it would be great @fsilent@xiarixiaoyao

@fsilent

Copy link
Copy Markdown
ContributorAuthor

LGTM as well, though I am still curious to understand in detail how does this interface SelfDescribingInputFormatInterface help in achieving column type evolution. I tried searching for docs, but could not find anything. If you can point me to relevant docs, it would be great @fsilent@xiarixiaoyao

In hive FetechOpertor, will judge needConversion by the inputformot whether implement SelfDescribingInputFormatInterface
image

@nsivabalannsivabalan added the release-0.12.2 Patches targetted for 0.12.2 label Dec 6, 2022
@codopecodope removed the release-0.12.2 Patches targetted for 0.12.2 label Dec 7, 2022
<artifactId>hive-common</artifactId>
<version>${hive.version}</version>
</dependency>
<dependency>

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.

Can you remove this dependency as the change is in hudi-hadoop-mr package and hive-exec is already defined as dependency in hudi-hadoop-mr /pom.xml

</dependency>

<!-- Hive -->
<dependency>

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.

This is still showing up. Can you remove this dependency as the change is in hudi-hadoop-mr package and hive-exec is already defined as dependency in hudi-hadoop-mr /pom.xml

@bvaradar

Copy link
Copy Markdown
Contributor

@fsilent : Thanks a lot for the PR. Can you fix the package dependencies and also add a jira ticket in the description.

@github-actionsgithub-actionsBot added the size:S PR with lines of changes in (10, 100] label Feb 26, 2024

@yihuayihua 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.

@bvaradar is this change still needed?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

engine:hiveHive integrationpriority:highSignificant impact; potential bugssize:SPR with lines of changes in (10, 100]

Projects

Status: 🏗 Under discussion

Development

Successfully merging this pull request may close these issues.

10 participants

@fsilent@xiarixiaoyao@leesf@hudi-bot@pratyakshsharma@bvaradar@yihua@nsivabalan@xushiyan@codope