Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HUDI-5400] Fix read issues when Hudi-FULL schema evolution is not enabled #7480

Merged
merged 7 commits into from
Dec 24, 2022

Conversation

voonhous
Copy link
Member

@voonhous voonhous commented Dec 16, 2022

Change Logs

Prior to Hudi's FULL Schema evolution (HFSE) support, Hudi relies on Avro's schema-resolution to perform schema evolution.

The exhaustive list of permitted schema-changes that Avro's schema-resolution allows for can be found here:
https://avro.apache.org/docs/1.10.2/spec.html#Schema+Resolution

A summary of the type changes is listed down below:

Supported cast conversions:
 - Integer => Long, Float, Double, Decimal*, String*
 - Long => Float, Double, Decimal*, String*
 - Float => Double, Decimal*, String*
 - Double => Decimal*, String*
 - Decimal => Decimal*, String*
 - String => Byte, Decimal*, Date*
 - Byte=> String
 - Date => String*

*type conversions that are supported by HFSE, but not in Native Avro's schema-resolution

The current write execution flow is as such:

  1. deduceWriterSchema to check if the incoming schema is compatible with table's schema (via org.apache.hudi.avro.AvroSchemaCompatibility.ReaderWriterCompatibilityChecker#calculateCompatibility)
  2. deduceWriterSchema's validation is an adaptation of Avro's schema compatibility check (org.apache.avro.SchemaCompatibility.ReaderWriterCompatiblityChecker#calculateCompatibility); if Avro permits such operation, allow execution to proceed
  3. As such if there are implicit schema changes which are compatible with Avro's schema resolution feature, HFSE does not need to be enabled
  4. If writer is writing to a different filegroup, this filegroup will be written with a new schema; while existing filegroups that are not written to will contain the old schema

When reading:

  1. When reading, the same schema will be used for all the filegroup since nothing is written to .schema.
  2. Parquet reader will throw errors due to type mismatches when reading

This PR fixes the Spark-read issues when implicit schema changes are made without enabling HFSE.

The scope of this fix is limited to Spark-Read + Spark-Write.

TODO: Check if issue exists in Flink reader/writer [WIP; WILL CREATE ANOTHER PR]
TODO: Implement the same changes to Spark2.4 [DONE; INCLUDED IN THIS PR]

Impact

None; no public APIs changed.

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

low

Documentation Update

Describe any necessary documentation update if there is any new feature, config, or user-facing change

  • The config description must be updated if new configs are added or the default value of the configs are changed
  • Any new feature or user-facing change requires updating the Hudi website. Please create a Jira ticket, attach the
    ticket number here and follow the instruction to make
    changes to the website.

Contributor's checklist

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

@voonhous voonhous changed the title Fix read issues when Hudi-FULL schema evolution is not enabled [HUDI-5400] Fix read issues when Hudi-FULL schema evolution is not enabled Dec 16, 2022
@xiarixiaoyao xiarixiaoyao self-assigned this Dec 16, 2022
@xiarixiaoyao
Copy link
Contributor

@voonhous
nice work,will take a look at the weekend

@xiarixiaoyao
Copy link
Contributor

@voonhous
Maybe we need a parameter to control this feature, not all tables need to follow this logic

@voonhous
Copy link
Member Author

voonhous commented Dec 20, 2022

@voonhous Maybe we need a parameter to control this feature, not all tables need to follow this logic

Hmmm, CMIIW, Hudi has been relying on ASR for schema resolution since hudi-0.7. As such, I was under the impression that this should be a default behaviour.

Nonetheless, a configuration key can be introduced where in this behaviour is enabled by default.

However, validation will need to be performed such that the choice between ASR/HFSE is mutually exclusive. i.e. if ASR is enabled, HFSE should be disabled and vice-versa. WDYT?

@@ -228,7 +228,24 @@ class Spark32PlusHoodieParquetFileFormat(private val shouldAppendPartitionValues

SparkInternalSchemaConverter.collectTypeChangedCols(querySchemaOption.get(), mergedInternalSchema)
} else {
new java.util.HashMap()
val implicitTypeChangeInfo: java.util.Map[Integer, Pair[DataType, DataType]] = new java.util.HashMap()
Copy link
Contributor

Choose a reason for hiding this comment

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

pls extract a function

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed with the latest commit

@xiarixiaoyao
Copy link
Contributor

Looks good.
maybe we also need a pr for flink

@voonhous
Copy link
Member Author

@voonhous Maybe we need a parameter to control this feature, not all tables need to follow this logic

Hmmm, CMIIW, Hudi has been relying on ASR for schema resolution since hudi-0.7. As such, I was under the impression that this should be a default behaviour.

Nonetheless, a configuration key can be introduced where in this behaviour is enabled by default.

However, validation will need to be performed such that the choice between ASR/HFSE is mutually exclusive. i.e. if ASR is enabled, HFSE should be disabled and vice-versa. WDYT?

@xiarixiaoyao I looked at the code and realised that there is no way validate configuration values based on other configuration values.

I wanted to add a AVRO_SCHEMA_RESOLUTION_ENABLE configuration key with the description:

Enable support for schema evolution using Avro's Schema Resolution (ASR). This configuration is mutually exclusive to Hudi's Full/Comprehensive Schema Evolution (HFSE) feature via the configuration key (hoodie.schema.on.read.enable). 

The choice between ASR/HFSE is mutually exclusive. i.e. if ASR is enabled, HFSE should be disabled and vice-versa. 

HFSE will take precedence over ASR. i.e. Enabling both HFSE and ASR will cause Hudi to default to HFSE for schema evolution.

Given that this is the intended behaviour and lack of configuration validation, I see no benefit for introducing AVRO_SCHEMA_RESOLUTION_ENABLE.

Since SCHEMA_EVOLUTION_ENABLE will take precedence over AVRO_SCHEMA_RESOLUTION_ENABLE, I think we can rely on the former (SCHEMA_EVOLUTION_ENABLE) to determine if ASR should be used.

If SCHEMA_EVOLUTION_ENABLE is enabled, use HFSE, else, fallback to ASR.

WDYT?

@xiarixiaoyao
Copy link
Contributor

@voonhous Maybe we need a parameter to control this feature, not all tables need to follow this logic

Hmmm, CMIIW, Hudi has been relying on ASR for schema resolution since hudi-0.7. As such, I was under the impression that this should be a default behaviour.
Nonetheless, a configuration key can be introduced where in this behaviour is enabled by default.
However, validation will need to be performed such that the choice between ASR/HFSE is mutually exclusive. i.e. if ASR is enabled, HFSE should be disabled and vice-versa. WDYT?

@xiarixiaoyao I looked at the code and realised that there is no way validate configuration values based on other configuration values.

I wanted to add a AVRO_SCHEMA_RESOLUTION_ENABLE configuration key with the description:

Enable support for schema evolution using Avro's Schema Resolution (ASR). This configuration is mutually exclusive to Hudi's Full/Comprehensive Schema Evolution (HFSE) feature via the configuration key (hoodie.schema.on.read.enable). 

The choice between ASR/HFSE is mutually exclusive. i.e. if ASR is enabled, HFSE should be disabled and vice-versa. 

HFSE will take precedence over ASR. i.e. Enabling both HFSE and ASR will cause Hudi to default to HFSE for schema evolution.

Given that this is the intended behaviour and lack of configuration validation, I see no benefit for introducing AVRO_SCHEMA_RESOLUTION_ENABLE.

Since SCHEMA_EVOLUTION_ENABLE will take precedence over AVRO_SCHEMA_RESOLUTION_ENABLE, I think we can rely on the former (SCHEMA_EVOLUTION_ENABLE) to determine if ASR should be used.

If SCHEMA_EVOLUTION_ENABLE is enabled, use HFSE, else, fallback to ASR.

WDYT?
agree

@xiarixiaoyao
Copy link
Contributor

@voonhous
pls rebase code,
once ci pass,we can merge it .

@voonhous
Copy link
Member Author

@voonhous pls rebase code, once ci pass,we can merge it .

Done!

@voonhous
Copy link
Member Author

@xiarixiaoyao I have added support for Hudi tables that are schema-evolved via ASR for Spark2.4.

Can you please help to review the PR again?

Thank you!

@xiarixiaoyao
Copy link
Contributor

@voonhous
Thank you for your support for spark2.4, although I personally think we don't need to support 2.4.
let‘s extract buildImplicitSchemaChangeInfo and isDataTypeEqual to a helper class to reuse.

@voonhous
Copy link
Member Author

@voonhous Thank you for your support for spark2.4, although I personally think we don't need to support 2.4. let‘s extract buildImplicitSchemaChangeInfo and isDataTypeEqual to a helper class to reuse.

Done!

@yihua yihua added schema-and-data-types priority:critical production down; pipelines stalled; Need help asap. writer-core Issues relating to core transactions/write actions labels Dec 22, 2022
@xiarixiaoyao
Copy link
Contributor

@hudi-bot run azure

@hudi-bot
Copy link

CI report:

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

@xiarixiaoyao
Copy link
Contributor

@voonhous
Thanks for your contribution

@xiarixiaoyao xiarixiaoyao merged commit 64b814e into apache:master Dec 24, 2022
@voonhous voonhous deleted the HUDI-5400 branch January 4, 2023 04:04
nsivabalan pushed a commit to nsivabalan/hudi that referenced this pull request Mar 22, 2023
fengjian428 pushed a commit to fengjian428/hudi that referenced this pull request Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:critical production down; pipelines stalled; Need help asap. schema-and-data-types writer-core Issues relating to core transactions/write actions
Projects
Archived in project
4 participants