[HUDI-6086] Improve HiveSchemaUtil#generateCreateDDL With StringBuilder #8478

slfan1989 · 2023-04-17T11:27:48Z

Change Logs

JIRA: HUDI-6086
HiveSchemaUtil#generateCreateDDL code is not easy to read. With Danny's help, we decided to refactor this part of the code using StringBuilder. I have written detailed comments for each section of the code to make it more readable.

Before Refactor:

After Refactor:

Impact

none.

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

none.

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

slfan1989 · 2023-04-17T11:32:06Z

@danny0405 Can you help review this pr? Thank you very much!

danny0405 · 2023-04-18T09:10:11Z

hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/util/HiveSchemaUtil.java

+      + "<" + ROW_FORMAT + ">\n"
+      + "<" + LOCATION_BLOCK + ">"
+      + "TBLPROPERTIES (\n"
+      + "<" + PROPERTIES + ">)";


two concerns here:

Where does the antlr jar comes from? Do we need to package antlr jar explicitly

the antlr grammar template is not that straight-forward, should refactoring like using the stringBuilder with params be enough now?

@danny0405 Thank you very much for helping review the code!

Antlr was introduced into the Hudi project in [HUDI-4111] Bump ANTLR runtime version in Spark 3.x ([HUDI-4111] Bump ANTLR runtime version in Spark 3.x #5606), we can find the reference of antlr in the parent pom.xml

<antlr.version>4.8</antlr.version>

hudi-hive-sync references hive-exec, so we can use antlr directly.

Thank you for your question! I agree with your point of view, the antlr grammar template is not that straight-forward. But for generating sql, I think we can use it, because the template can better describe the components of sql.

HiveSchemaUtil#CREATE_TABLE_TEMPLATE

private static final String CREATE_TABLE_TEMPLATE = "CREATE <" + EXTERNAL + ">TABLE <if(" + DATABASE_NAME + ")>`<" + DATABASE_NAME + ">`.<endif>" + "`<" + TABLE_NAME + ">`(\n" + "<" + LIST_COLUMNS + ">)\n" + "<" + PARTITIONS + ">\n" + "<" + BUCKETS + ">\n" + "<" + ROW_FORMAT + ">\n" + "<" + LOCATION_BLOCK + ">" + "TBLPROPERTIES (\n" + "<" + PROPERTIES + ">)";

Through this template, we can know the elements of the sql to be generated, such as the name of the table, the name of the database, and various attributes of the table. It is easier to read this part of the code.

We will not use very complicated templates, just to improve code readability.

Not a fan of antlr, this introduces unnecessary complexity and dependency of the antlr jar, how about we refactor the code using just a string builder, each sub-clause can be built separately.

I agree with you. I will refactor this part of code with string builder.

@danny0405 Thank you very much for helping to review the code and providing suggestions for improvement! I have refactored this part of the code using StringBuilder, as we discussed, to improve its readability. I also added some comments to provide additional context and clarification. Sorry for the delayed response.

When you have time, could you please review this PR again? Thank you very much!

slfan1989 · 2023-04-28T11:04:12Z

@hudi-bot run azure

danny0405 · 2023-05-02T07:32:43Z

Thanks for the contribution, I have reviewed and created a patch:
6086.patch.zip

I saw some sub-claused locations are changed, like the LOCATION and CLUSTETERED BY, is that as expected?

…der.

danny0405 · 2023-05-02T06:38:18Z

hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/ddl/QueryBasedDDLExecutor.java

+   * @param serdeClass serdeClass.
+   * @param serdeProperties serdeProperties.
+   * @param tableProperties tableProperties.
+   */


Maybe just remove the patam docs if there is no much to address:

/** * Create a table with the given params. */

Thank you for your suggestion! I will simplify the comment information.

slfan1989 · 2023-05-03T10:31:03Z

@danny0405

Thank you very much for your reminder! I carefully readed the code again and modify the code.

The concatenation order is consistent between the original and modified versions, both of which consist of 7 steps.

Append Create Table
Append Columns
Append Partitions
Append Bucket
Append Row Format
Append Location
Append TblProperties

Original Version.

Modified Version.

slfan1989 · 2023-05-03T10:35:37Z

I saw some sub-claused locations are changed, like the LOCATION and CLUSTETERED BY, is that as expected?

@danny0405 Thank you for your thorough review! The modification to the LOCATION code should be as expected, but there were some issues with the modification to the BUCKET code. I have reverted the BUCKET code back to its original state.

danny0405 · 2023-05-04T05:25:52Z

I saw some sub-claused locations are changed, like the LOCATION and CLUSTETERED BY, is that as expected?

@danny0405 Thank you for your thorough review! The modification to the LOCATION code should be as expected, but there were some issues with the modification to the BUCKET code. I have reverted the BUCKET code back to its original state.

We better have a straight-forward string paradigm of what the DDL looks like before and after the change, in case there are some discrepencies.

slfan1989 · 2023-05-04T07:42:22Z

I will add a set of unit tests to compare the generated SQL before and after modification.

danny0405 · 2023-05-12T08:08:18Z

hudi-sync/hudi-hive-sync/src/test/java/org/apache/hudi/hive/util/TestHiveSchemaUtil.java

+    String expectedCreateDataBaseSQL = "CREATE DATABASE IF NOT EXISTS `test_database`";
+    String testDataBase = HiveSchemaUtil.generateCreateDataBaseDDL("test_database");
+    assertEquals(expectedCreateDataBaseSQL, testDataBase);
+  }


Can we also have a test case for create table?

Thanks for your suggestion, I will submit the test code as soon as possible.

hudi-bot · 2023-05-22T14:10:12Z

CI report:

1b207df UNKNOWN
708a1ba UNKNOWN
4d4a079 Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

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

danny0405 · 2023-05-23T08:37:14Z

hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/ddl/QueryBasedDDLExecutor.java


+  /**
+   * Create a table with the given params.
+   */


Is the doc correct?

danny0405 · 2023-05-23T08:37:21Z

hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/ddl/QueryBasedDDLExecutor.java


+  /**
+   * Create a table with the given params.
+   */


danny0405 · 2023-05-23T08:38:04Z

hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/util/HiveSchemaUtil.java


-  public static String generateSchemaString(MessageType storageSchema, List<String> colsToSkip, boolean supportTimestamp) throws IOException {
+  /**
+   * Generates the Column DDL string for creating a Hive table from the given schema.


I love the original document that we add a SQL statement demo for the method, can we keep that?

danny0405 changed the title ~~[HUDI-6086]. Improve HiveSchemaUtil#generateCreateDDL With ST.~~ [HUDI-6086] Improve HiveSchemaUtil#generateCreateDDL With ST Apr 18, 2023

danny0405 reviewed Apr 18, 2023

View reviewed changes

danny0405 self-assigned this Apr 18, 2023

danny0405 added type:refactor Code refactoring and cleanup priority:medium Moderate impact; usability gaps labels Apr 18, 2023

slfan1989 requested a review from danny0405 April 27, 2023 02:41

slfan1989 changed the title ~~[HUDI-6086] Improve HiveSchemaUtil#generateCreateDDL With ST~~ [HUDI-6086] Improve HiveSchemaUtil#generateCreateDDL With StringBuilder Apr 27, 2023

slfan1989 force-pushed the HUDI-6086 branch from 1b207df to dce3b83 Compare April 28, 2023 02:40

[HUDI-6086]. Improve HiveSchemaUtil#generateCreateDDL With StringBuil…

a389aad

…der.

slfan1989 force-pushed the HUDI-6086 branch from 24db1b1 to a389aad Compare May 3, 2023 08:30

slfan1989 and others added 2 commits May 3, 2023 16:31

Merge branch 'apache:master' into HUDI-6086

1639d63

[HUDI-6086]. Fix CheckStyle.

708a1ba

danny0405 reviewed May 3, 2023

View reviewed changes

slfan1989 added 2 commits May 3, 2023 17:33

[HUDI-6086]. Fix CheckStyle.

e0f28bb

[HUDI-6086]. Fix CheckStyle.

2f56824

Merge branch 'apache:master' into HUDI-6086

129cc30

danny0405 reviewed May 12, 2023

View reviewed changes

slfan1989 and others added 2 commits May 21, 2023 09:49

Merge branch 'apache:master' into HUDI-6086

ae2265f

[HUDI-6086]. Fix CheckStyle.

4d4a079

danny0405 reviewed May 23, 2023

View reviewed changes

github-actions bot added the size:M PR with lines of changes in (100, 300] label Feb 26, 2024

hudi-bot mentioned this pull request Dec 9, 2025

Improve HiveSchemaUtil#generateCreateDDL With StringBuilder. #15906

Open

[HUDI-6086] Improve HiveSchemaUtil#generateCreateDDL With StringBuilder #8478

Are you sure you want to change the base?

[HUDI-6086] Improve HiveSchemaUtil#generateCreateDDL With StringBuilder #8478

Uh oh!

Conversation

slfan1989 commented Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Logs

Impact

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

Documentation Update

Contributor's checklist

Uh oh!

slfan1989 commented Apr 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slfan1989 Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slfan1989 commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danny0405 commented May 2, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slfan1989 commented May 3, 2023

Uh oh!

slfan1989 commented May 3, 2023

Uh oh!

danny0405 commented May 4, 2023

Uh oh!

slfan1989 commented May 4, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hudi-bot commented May 22, 2023

CI report:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

slfan1989 commented Apr 17, 2023 •

edited

Loading

slfan1989 Apr 19, 2023 •

edited

Loading

slfan1989 commented Apr 28, 2023 •

edited

Loading