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

Add explicit check for modifying data in delta table with CDF enabled #14470

Conversation

homar
Copy link
Member

@homar homar commented Oct 5, 2022

Description

Add explicit check for modifying data in delta table with CDF enabled.
Inserts could be allowed as they don't have to do anything special to support CDF but they are blocked by writer version check.

Non-technical explanation

Release notes

(x) This is not user-visible or docs only and no release notes are required.

@cla-bot cla-bot bot added the cla-signed label Oct 5, 2022
@homar homar force-pushed the homar/add_test_checking_that_writes_to_delta_table_with_cdf_fails branch from a9c893c to 677d2ca Compare October 5, 2022 11:14
@homar homar marked this pull request as ready for review October 5, 2022 11:14
@homar homar requested a review from findepi October 5, 2022 20:14
@homar homar force-pushed the homar/add_test_checking_that_writes_to_delta_table_with_cdf_fails branch 2 times, most recently from 0e0e7e2 to 5f87b1f Compare October 6, 2022 21:58
@ebyhr
Copy link
Member

ebyhr commented Oct 7, 2022

Sent #14508 within the repository.

@homar homar force-pushed the homar/add_test_checking_that_writes_to_delta_table_with_cdf_fails branch from 5f87b1f to e51007e Compare October 7, 2022 06:58
public static boolean changeDataFeedEnabled(MetadataEntry metadataEntry)
{
String enableChangeDataFeed = metadataEntry.getConfiguration().getOrDefault("delta.enableChangeDataFeed", "false");
return enableChangeDataFeed.equalsIgnoreCase("true");
Copy link
Member

Choose a reason for hiding this comment

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

Is java.lang.Boolean#parseBoolean what Delta is using?

Copy link
Member

Choose a reason for hiding this comment

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

This is Delta implementation:

  val CHANGE_DATA_FEED = buildConfig[Boolean](
    "enableChangeDataFeed",
    "false",
    _.toBoolean,
    _ => true,
    "needs to be a boolean.",
    alternateConfs = Seq(CHANGE_DATA_FEED_LEGACY))

  def toBoolean: Boolean = parseBoolean(toString)

  private def parseBoolean(s: String): Boolean =
    if (s != null) s.toLowerCase match {
      case "true" => true
      case "false" => false
      case _ => throw new IllegalArgumentException("For input string: \""+s+"\"")
    }
    else
      throw new IllegalArgumentException("For input string: \"null\"")

"TBLPROPERTIES (delta.enableChangeDataFeed = true)");

assertQueryFailure(() -> onTrino().executeQuery("INSERT INTO delta.default." + tableName + " VALUES (1, 2)"))
.hasMessageMatching(".* Table .* requires Delta Lake writer version 4 which is not supported");
Copy link
Member

Choose a reason for hiding this comment

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

So it was blocked before the change as well and the change just introduces an explicit check with an explicit message.

Let's have a different commit and PR title then

@homar homar force-pushed the homar/add_test_checking_that_writes_to_delta_table_with_cdf_fails branch from e51007e to c28b844 Compare October 7, 2022 08:05
@homar homar changed the title Block modifying data in delta tables with CDF enabled Add explicit check for modifying data in delta table with CDF enabled Oct 7, 2022
@homar
Copy link
Member Author

homar commented Oct 7, 2022

maven checks were killed because they were super slow, doesn't look like anything actionable

@ebyhr
Copy link
Member

ebyhr commented Oct 7, 2022

Could you rebase on upstream to resolve conflicts?

@homar homar force-pushed the homar/add_test_checking_that_writes_to_delta_table_with_cdf_fails branch from e7d2288 to 875a541 Compare October 7, 2022 22:11
@findepi findepi merged commit 17bf26c into trinodb:master Oct 10, 2022
@github-actions github-actions bot added this to the 400 milestone Oct 10, 2022
@colebow
Copy link
Member

colebow commented Oct 11, 2022

Could you clarify what the user impact of this is for the release note?

It seems to me like this would always fail (due to that writer version check), but now it's failing with better error messaging?

@ebyhr
Copy link
Member

ebyhr commented Oct 11, 2022

It seems to me like this would always fail (due to that writer version check), but now it's failing with better error messaging?

Correct. There's no need to mention in a release note.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants