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-6924] Fix hoodie table config not wok in table properties #9836

Merged
merged 2 commits into from
Oct 17, 2023

Conversation

wecharyu
Copy link
Contributor

@wecharyu wecharyu commented Oct 9, 2023

Change Logs

Fix the bug that hoodie table config not work in table properties. Including:

  1. Combine mapTableConfigsToSqlOptions and mapDataSourceWriteOptionsToSqlOptions to mapHoodieOptionsToSqlOptions in HoodieOptionConfig.
  2. Remove unused code in HoodieOptionConfig.

Impact

Make hoodie.table.* configs take effect.

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

@danny0405
Copy link
Contributor

@boneanxs Do you have intreast to review this PR?

@@ -199,7 +184,7 @@ object HoodieOptionConfig {

// extract primaryKey, preCombineField, type options
def extractSqlOptions(options: Map[String, String]): Map[String, String] = {
val sqlOptions = mapTableConfigsToSqlOptions(options)
val sqlOptions = mapHoodieOptionsToSqlOptions(options)
Copy link
Contributor

Choose a reason for hiding this comment

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

mapHoodieConfigsToSqlOptions should be more accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense.

* @param options
* @return
*/
def getTableType(options: Map[String, String]): String = {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to delete this

@hudi-bot
Copy link

CI report:

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

Copy link
Contributor

@boneanxs boneanxs left a comment

Choose a reason for hiding this comment

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

+1 for this, this is actually a bug that hoodie tableconfig won't take effect if user set it.

But we need to note that there's a behavior change if user both set writeConfig & tableConfig, before only writeConfigs take effect, but now tableConfig has priority than writeConfig(Thinking this should be minor since user usually won't set both).

@wecharyu
Copy link
Contributor Author

@hudi-bot run azure

@danny0405 danny0405 merged commit 7c79ebe into apache:master Oct 17, 2023
27 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants