Skip to content

Add support for partition schema evolution for HUDI tables#16348

Merged
zhenxiao merged 1 commit intoprestodb:masterfrom
imjalpreet:HUDISchemaEvolution
Jul 14, 2021
Merged

Add support for partition schema evolution for HUDI tables#16348
zhenxiao merged 1 commit intoprestodb:masterfrom
imjalpreet:HUDISchemaEvolution

Conversation

@imjalpreet
Copy link
Copy Markdown
Member

@imjalpreet imjalpreet commented Jun 28, 2021

This is a follow-up PR for #16011. This PR enables partition schema evolution for HUDI tables.

== RELEASE NOTES ==

Hive Changes
* Add support for allowing to match columns between table and partition schemas by names for HUDI tables. This is enabled when configuration property ``hive.parquet.use-column-names`` or the hive catalog session property ``parquet_use_column_names`` is set to true. By default they are mapped by index.

@imjalpreet imjalpreet requested a review from zhenxiao June 28, 2021 09:03
zhenxiao
zhenxiao reviewed Jun 29, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@zhenxiao zhenxiao left a comment

Choose a reason for hiding this comment

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

one minor thing

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitManager.java Outdated
Copy link
Copy Markdown
Collaborator

@zhenxiao zhenxiao Jun 29, 2021

Choose a reason for hiding this comment

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

how about:
s/finalHiveStorageFormat/resolvedStorageFormat/g

Copy link
Copy Markdown
Member Author

@imjalpreet imjalpreet Jul 8, 2021

Choose a reason for hiding this comment

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

Sure, made the requested change.

zhenxiao
zhenxiao reviewed Jun 29, 2021
View reviewed changes
presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitManager.java Outdated
Copy link
Copy Markdown
Collaborator

@zhenxiao zhenxiao Jun 29, 2021

Choose a reason for hiding this comment

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

shall we check session property or configuration here? according to the release note:
This is enabled when configuration property hive.parquet.use-column-names or the hive catalog session property parquet_use_column_names is set to true. By default they are mapped by index.

Copy link
Copy Markdown
Member Author

@imjalpreet imjalpreet Jul 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@zhenxiao Line 392 and Line 393 are required in any case. Only next couple of lines are not mandatory if hive.parquet.use-column-names is not set to true. But in any case value of this variable is only used in one method getTableToPartitionMapping which has the required check at line 528.

Anyways, I have added a check here as well, let me know if you feel it is not necessary.

Copy link
Copy Markdown
Collaborator

@zhenxiao zhenxiao Jul 13, 2021

Choose a reason for hiding this comment

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

looks nice

imjalpreet
imjalpreet commented Jul 8, 2021
View reviewed changes
Copy link
Copy Markdown
Member Author

@imjalpreet imjalpreet left a comment

Choose a reason for hiding this comment

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

@zhenxiao Sorry for the delay, I was away for the past few days.

I have made the requested changes, please let me know your views.

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitManager.java Outdated
Copy link
Copy Markdown
Member Author

@imjalpreet imjalpreet Jul 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@zhenxiao Line 392 and Line 393 are required in any case. Only next couple of lines are not mandatory if hive.parquet.use-column-names is not set to true. But in any case value of this variable is only used in one method getTableToPartitionMapping which has the required check at line 528.

Anyways, I have added a check here as well, let me know if you feel it is not necessary.

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitManager.java Outdated
Copy link
Copy Markdown
Member Author

@imjalpreet imjalpreet Jul 8, 2021

Choose a reason for hiding this comment

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

Sure, made the requested change.

@imjalpreet imjalpreet force-pushed the HUDISchemaEvolution branch from dc3c96a to bc530d9 Compare July 8, 2021 08:52
@imjalpreet imjalpreet requested a review from zhenxiao July 8, 2021 08:56
zhenxiao
zhenxiao approved these changes Jul 13, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@zhenxiao zhenxiao left a comment

Choose a reason for hiding this comment

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

thank you, @imjalpreet
looks good. once minor issue

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitManager.java Outdated
Copy link
Copy Markdown
Collaborator

@zhenxiao zhenxiao Jul 13, 2021

Choose a reason for hiding this comment

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

looks nice

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitManager.java
StorageFormat storageFormat = table.getStorage().getStorageFormat();
Optional<HiveStorageFormat> hiveStorageFormat = getHiveStorageFormat(storageFormat);

Optional<HiveStorageFormat> resolvedHiveStorageFormat;
Copy link
Copy Markdown
Collaborator

@zhenxiao zhenxiao Jul 13, 2021

Choose a reason for hiding this comment

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

how about:

Optional<HiveStorageFormat> resolvedHiveStorageFormat = hiveStorageFormat;
if (isUseParquetColumnNames(session)) {
 ...
}

Copy link
Copy Markdown
Member Author

@imjalpreet imjalpreet Jul 13, 2021

Choose a reason for hiding this comment

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

@zhenxiao The variable resolvedHiveStorageFormat is being used in a lambda function so it has to be a final or an effectively final variable. If I set it before the if condition it won't remain an effectively final variable as it's value will change for the second time inside the if block.

Due to this reason I had to use an else block.

Let me know if you think I can improve it some other way.

Copy link
Copy Markdown
Collaborator

@zhenxiao zhenxiao Jul 14, 2021

Choose a reason for hiding this comment

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

I might miss something. which lambda function is used for resolvedHiveStorageFormat? seems it is only used in getTableToPartitionMapping?

imjalpreet
imjalpreet commented Jul 14, 2021
View reviewed changes
Copy link
Copy Markdown
Member Author

@imjalpreet imjalpreet left a comment

Choose a reason for hiding this comment

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

@zhenxiao I have left a comment at the line where the lambda function starts. Can you have a look and let me know in case of any concerns.

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitManager.java
}

Iterable<List<HivePartition>> partitionNameBatches = partitionExponentially(hivePartitions, minPartitionBatchSize, maxPartitionBatchSize);
Iterable<List<HivePartitionMetadata>> partitionBatches = transform(partitionNameBatches, partitionBatch -> {
Copy link
Copy Markdown
Member Author

@imjalpreet imjalpreet Jul 14, 2021

Choose a reason for hiding this comment

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

@zhenxiao I was talking about this. You are right that resolvedHiveStorageFormat is only being used in the method getTableToPartitionMapping but that method is being called from this lambda function. The call is on the line 466 in the updated code.

Copy link
Copy Markdown
Collaborator

@zhenxiao zhenxiao Jul 14, 2021

Choose a reason for hiding this comment

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

get it. you are correct

zhenxiao
zhenxiao approved these changes Jul 14, 2021
View reviewed changes
presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitManager.java
}

Iterable<List<HivePartition>> partitionNameBatches = partitionExponentially(hivePartitions, minPartitionBatchSize, maxPartitionBatchSize);
Iterable<List<HivePartitionMetadata>> partitionBatches = transform(partitionNameBatches, partitionBatch -> {
Copy link
Copy Markdown
Collaborator

@zhenxiao zhenxiao Jul 14, 2021

Choose a reason for hiding this comment

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

get it. you are correct

@zhenxiao zhenxiao merged commit 5090612 into prestodb:master Jul 14, 2021
@swapsmagic swapsmagic mentioned this pull request Jul 19, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zhenxiao zhenxiao zhenxiao approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@imjalpreet @zhenxiao