[HUDI-7622] Optimize HoodieTableSource's sanity check #11031

zhuanshenbsj1 · 2024-04-16T14:44:23Z

Change Logs

Existing exceptions cannot know which table and which specific columns the exception is.

Modify as follows：

Printing wrong columns name and table name in exception for MergeOnReadTableState#getRequiredPositions，and advance the exception from the operator execution stage to the operator initialization stage

check columns

check pks

Impact

Describe any public API or user-facing feature change or any performance impact.

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

If medium or high, explain what verification was done to mitigate the risks.

Documentation Update

Describe any necessary documentation update if there is any new feature, config, or user-facing change. If not, put "none".

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

danny0405 · 2024-04-17T00:46:12Z

...asource/hudi-flink/src/main/java/org/apache/hudi/table/format/mor/MergeOnReadTableState.java

+            .toArray();
+    if (!expColumns.isEmpty()) {
+      throw new HoodieException("Column(s) " + String.join(", ", expColumns) + " does not exists in the hudi table " + this.tableName + ".");
+    }


Column(s) [$col_a, $col_b, $col_c ...] does not exist in the table $tableName.

...asource/hudi-flink/src/main/java/org/apache/hudi/table/format/mor/MergeOnReadTableState.java

danny0405 · 2024-04-23T01:27:08Z

hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/table/HoodieTableFactory.java

@@ -86,12 +84,14 @@ public DynamicTableSource createDynamicTableSource(Context context) {
    setupTableOptions(conf.getString(FlinkOptions.PATH), conf);
    ResolvedSchema schema = context.getCatalogTable().getResolvedSchema();
    setupConfOptions(conf, context.getObjectIdentifier(), context.getCatalogTable(), schema);
-    return new HoodieTableSource(
+    HoodieTableSource source = new HoodieTableSource(
        schema,
        path,
        context.getCatalogTable().getPartitionKeys(),
        conf.getString(FlinkOptions.PARTITION_DEFAULT_NAME),


Let's keep the sanity check in the HoodieTableFactory?

If according to what you said, Metadataclient needs to be initialized in the factory(Hoodie source sanity check need). It seems more reasonable to initialize in source?

Metadataclient needs to be initialized in the factory

That's okay, we already do that for the sink sanity check of table config.

@danny0405 cc

danny0405 · 2024-04-26T08:33:46Z

hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/table/HoodieTableSource.java

@@ -518,7 +499,7 @@ private MergeOnReadInputFormat mergeOnReadInputFormat(
        tableAvroSchema.toString(),
        AvroSchemaConverter.convertToSchema(requiredRowType).toString(),
        inputSplits,
-        conf.getString(FlinkOptions.RECORD_KEY_FIELD).split(","));
+        OptionsResolver.getRecordKeyField(conf));


The change is way too complicated, can you re-illustrate the issue again? What is the use case from user and what the correct behavior is expected?

The table created by the upstream write (recorded in the existing metadata) do not match the columns configured by the downstream stream read. For example, some columns do not exist, resulting in the columns not be found.
-> Verification failed, throwing exception

The recordkey configuration does not exist
-> Verification failed, throwing exception

Case problem. The columns created based on calsite in the upstream are all lowercase. If there are uppercase in the downstream, such as "eventTime", the columns will not be found.
->Uniformly converted to lowercase

Case problem. The columns created based on calsite in the upstream are all lowercase. If there are uppercase in the downstream, such as "eventTime", the columns will not be found.
->Uniformly converted to lowercase

This is not expected to be handled by Hudi, I think. At least, from the catalog layer, we should make the case-sensitivity agnostic to specific engines.

The table created by the upstream write (recorded in the existing metadata) do not match the columns configured by the do

In HoodieTableFactory#createDynamicTableSource, add a sanity check for the catalog table resolved schema and the existing Hudi table schema, that should be enough I guess. Similiar with the primary key definition.

@danny0405 cc~

danny0405 · 2024-04-28T03:36:12Z

Thanks for the contribuiton, here is a patch for the fix:
7622.patch.zip

It would be greate if you can help to add some UTs.

zhuanshenbsj1 · 2024-05-16T07:48:15Z

Thanks for the contribuiton, here is a patch for the fix: 7622.patch.zip

It would be greate if you can help to add some UTs.

Made some adjustments:

Create a new utility-class called SanityCheckUtil for validation, and move the checkKeygenGenerator and checkPreCombineKey methods from StreamerUtil to this class.
Instantiate the MetaClient operation is heavy, so the Hudi source constructor adds a metaclient parameter to avoid multiple initializations for metaclient.
Merge source and sink validation into same function, differentiating them by checking if the metaclient is null.
Source needs to validate whether the columns and recordkey are consistent with the metadata.
All exception prints after validation should include the table name.
Adjust some ut for change.

danny0405 · 2024-05-16T07:54:54Z

hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/util/SanityCheckUtil.java

+/**
+ * Utilities for HoodieTableFactory sanity check.
+ */
+public class SanityCheckUtil {


Rename class name to SanityChecks.

Rename class name to SanityChecks.

Done.

danny0405 · 2024-05-16T07:56:16Z

hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/table/HoodieTableFactory.java

    return new HoodieTableSource(
        schema,
        path,
        context.getCatalogTable().getPartitionKeys(),
        conf.getString(FlinkOptions.PARTITION_DEFAULT_NAME),
-        conf);
+        conf,
+        metaClient);


The metaClient initialization is costly but the table source only happens once in Job Graph compile time, let's not reuse here to reduce complexity.

The metaClient initialization is costly but the table source only happens once in Job Graph compile time, let's not reuse here to reduce complexity.

Done.

danny0405 · 2024-05-17T04:05:38Z

hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/table/HoodieTableFactory.java

@@ -86,6 +86,8 @@ public DynamicTableSource createDynamicTableSource(Context context) {
    setupTableOptions(conf.getString(FlinkOptions.PATH), conf);
    ResolvedSchema schema = context.getCatalogTable().getResolvedSchema();
    setupConfOptions(conf, context.getObjectIdentifier(), context.getCatalogTable(), schema);
+    HoodieTableMetaClient metaClient = StreamerUtil.metaClientForReader(conf, HadoopConfigurations.getHadoopConf(conf));
+    SanityChecksUtil.sanitCheck(conf, schema, metaClient);


Move the instantiation of meta client into the SanityCheckUtil. And can we rename the class to SanityChecks

danny0405 · 2024-05-17T09:08:09Z

hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/util/SanityChecks.java

    checkTableType(conf);
    List<String> schemaFields = schema.getColumnNames();
-    if (metaClient != null) {
+    if (checkMetaData) {
+      HoodieTableMetaClient metaClient = StreamerUtil.metaClientForReader(conf, HadoopConfigurations.getHadoopConf(conf));
      List<String> latestTablefields = StreamerUtil.getLatestTableFields(metaClient);
      if (latestTablefields != null) {


The logic for sink has been changed with this patch. We have this code for the original sink:

if (!OptionsResolver.isAppendMode(conf)) { checkRecordKey(conf, schema); }

I put this logic into function checkRecordKey, and both the source and sink need to be checked.

public static void checkRecordKey(Configuration conf,List<String> existingFields) { if (OptionsResolver.isAppendMode(conf)) { return; } .... }

And also do this in function checkIndexType.

public static void checkIndexType(Configuration conf) { if (OptionsResolver.isAppendMode(conf)) { return; } .... }

danny0405 · 2024-05-22T00:52:16Z

hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/table/HoodieTableFactory.java

    setupConfOptions(conf, context.getObjectIdentifier(), context.getCatalogTable(), schema);
    setupSortOptions(conf, context.getConfiguration());
+    SanityChecks.sanitCheck(conf, schema, false);


Should move the line before 105.

hudi-bot · 2024-05-28T20:47:49Z

CI report:

e159472 UNKNOWN
e1913de UNKNOWN
b636f39 Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

@hudi-bot run azure re-run the last Azure build

github-actions bot added the size:S PR with lines of changes in (10, 100] label Apr 16, 2024

danny0405 reviewed Apr 17, 2024

View reviewed changes

...asource/hudi-flink/src/main/java/org/apache/hudi/table/format/mor/MergeOnReadTableState.java Outdated Show resolved Hide resolved

zhuanshenbsj1 force-pushed the HUDI-7622 branch 6 times, most recently from 5ef17f7 to a7270a9 Compare April 18, 2024 01:46

github-actions bot added size:L PR with lines of changes in (300, 1000] and removed size:S PR with lines of changes in (10, 100] labels Apr 22, 2024

zhuanshenbsj1 force-pushed the HUDI-7622 branch 3 times, most recently from a487acf to 0b3c631 Compare April 22, 2024 16:14

danny0405 reviewed Apr 23, 2024

View reviewed changes

zhuanshenbsj1 force-pushed the HUDI-7622 branch 3 times, most recently from e667c4b to 20af652 Compare April 26, 2024 06:55

danny0405 reviewed Apr 26, 2024

View reviewed changes

zhuanshenbsj1 force-pushed the HUDI-7622 branch from 20af652 to e10e529 Compare April 28, 2024 02:07

zhuanshenbsj1 changed the title ~~[MINOR] Optimization function MergeOnReadTableState#getRequiredPositions~~ [HUDI-7622] Add sanity check for HoodieTableSource Apr 28, 2024

zhuanshenbsj1 force-pushed the HUDI-7622 branch from e10e529 to b3ed3ad Compare April 28, 2024 02:29

zhuanshenbsj1 force-pushed the HUDI-7622 branch 2 times, most recently from 14a352d to a7ab936 Compare May 16, 2024 07:41

zhuanshenbsj1 changed the title ~~[HUDI-7622] Add sanity check for HoodieTableSource~~ [HUDI-7622] Optimize HoodieTableSource's sanity check May 16, 2024

danny0405 reviewed May 16, 2024

View reviewed changes

zhuanshenbsj1 force-pushed the HUDI-7622 branch from a7ab936 to 0dac8e3 Compare May 16, 2024 08:09

zhuanshenbsj1 force-pushed the HUDI-7622 branch 2 times, most recently from f0936bc to 30f50eb Compare May 16, 2024 08:41

danny0405 reviewed May 17, 2024

View reviewed changes

zhuanshenbsj1 added 4 commits May 21, 2024 15:51

add source check

b1dff80

append no need to check index type

3afb16f

preassign table name for saint check print

27eb309

rename sanitycheck

6754515

zhuanshenbsj1 force-pushed the HUDI-7622 branch from 74e2af7 to 0453ba3 Compare May 21, 2024 07:52

remove ComputedColumn check & include metadata fields

b636f39

zhuanshenbsj1 force-pushed the HUDI-7622 branch from 0453ba3 to b636f39 Compare May 21, 2024 07:56

danny0405 reviewed May 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HUDI-7622] Optimize HoodieTableSource's sanity check #11031

[HUDI-7622] Optimize HoodieTableSource's sanity check #11031

zhuanshenbsj1 commented Apr 16, 2024 •

edited

danny0405 Apr 17, 2024

zhuanshenbsj1 Apr 17, 2024

danny0405 Apr 23, 2024

zhuanshenbsj1 Apr 23, 2024

danny0405 Apr 25, 2024

zhuanshenbsj1 Apr 26, 2024

danny0405 Apr 26, 2024

zhuanshenbsj1 Apr 26, 2024

danny0405 Apr 26, 2024

zhuanshenbsj1 Apr 28, 2024

danny0405 commented Apr 28, 2024

zhuanshenbsj1 commented May 16, 2024 •

edited

danny0405 May 16, 2024 •

edited

zhuanshenbsj1 May 16, 2024

danny0405 May 16, 2024

zhuanshenbsj1 May 16, 2024

danny0405 May 17, 2024

danny0405 May 17, 2024

zhuanshenbsj1 May 17, 2024

danny0405 May 22, 2024

hudi-bot commented May 28, 2024

[HUDI-7622] Optimize HoodieTableSource's sanity check #11031

Are you sure you want to change the base?

[HUDI-7622] Optimize HoodieTableSource's sanity check #11031

Conversation

zhuanshenbsj1 commented Apr 16, 2024 • edited

Change Logs

Impact

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

Documentation Update

Contributor's checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danny0405 commented Apr 28, 2024

zhuanshenbsj1 commented May 16, 2024 • edited

danny0405 May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hudi-bot commented May 28, 2024

CI report:

zhuanshenbsj1 commented Apr 16, 2024 •

edited

zhuanshenbsj1 commented May 16, 2024 •

edited

danny0405 May 16, 2024 •

edited