Allow enabling CDF using ALTER TABLE SET PROPERTIES

[homar]

Description

Based on #16165
Allows setting on and off change_data_feed_enabled property

Additional context and related issues

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

[krvikash]

Can we create a helper method to get the MetadataEntry? and we can use that helper method in appendTableEntries as well?

[homar]

I tried but it became messy, I will do it as another PR, ok ?

[krvikash]

Since we are now supporting additional property in ALTER TABLE ..., protocolEntry should be appended if present.

[homar]

So I've just checked, I did run this three operations:
CREATE TABLE student31 (id INT, name STRING, age INT) USING DELTA LOCATION XXXX
INSERT INTO student31 VALUES(1, 'homar', 13);
ALTER TABLE student31 SET TBLPROPERTIES (delta.enableChangeDataFeed = true)

The last one resulted in creation of 00000000000000000002.json.
And it has protocol entry that looks like this:

{
  "protocol": {
    "minReaderVersion": 1,
    "minWriterVersion": 4
  }
}

So I don't think there is a need to add changeDataFeed there

[krvikash]

Yes, right the protocol version will not get updated and use the previous protocol version.

My point was whether adding a protocol entry in the transaction log is necessary if the user has not set reader_version or writer_version property using ALTER TABLE ....

CC: @alexjo2144

[homar]

oh ok I see what you mean, so imho if we throw when writerVersion is too low then protocol version doesn't change here so there is no need to append it. i will change this

[krvikash]

testAlterTableWithUnsupportedProperties needs to be updated.

[krvikash]

Can we have tests with a combination of change_data_feed_enabled, ALTER_TABLE, and writer_version?

[krvikash]

Something like testCreateTableWithCdfEnabledAndUnsupportedWriterVersionFails, testCreateTableAsWithCdfPropertyWriterVersionNotUpgraded

[homar]

I modified the test I added and renamed it to testSettingChangeDataGeedEnabledProperty

[findinpath]

nit: bring configuration inside the subsequent if - this is where it is only used (at the moment).

[krvikash]

Please resolve conflicts. #16165 is merged now.

[krvikash]

IMO, I'd go with earlier implementation instead of adding methods in ProtocolEntry.

        Optional<ProtocolEntry> protocolEntry = Optional.empty();
        if (readerVersion.isPresent() || writerVersion.isPresent()) {
            protocolEntry = Optional.of(new ProtocolEntry(
                    readerVersion.orElse(currentProtocolEntry.getMinReaderVersion()),
                    writerVersion.orElse(currentProtocolEntry.getMinWriterVersion())));
       }

[homar]

ok, but i liked mine version ;)

[krvikash]

Can we keep this and verify the error does not include change_data_feed_enabled?

[krvikash]

I think this should work. since we are not enabling cdf. enabling cdf with writer_version less than 4 should throw exception.

[homar]

But in this case we don't set writer_version so it is set to 2, it is verified at the beginning of this test. I thought we agreed that we should not increase writer_version silently.

[krvikash]

yes right, we are not increasing writer_version silently.

change_data_feed_enabled = false is the same as not setting change_data_feed_enabled.

In my opinion, executing CREATE TABLE test (a INT) WITH (change_data_feed_enabled = false, writer_version = 3) or CREATE TABLE test (a INT) WITH (change_data_feed_enabled = false) does not result in an exception. Similarly, when running ALTER TABLE test SET PROPERTIES change_data_feed_enabled = false, exception should not be thrown, regardless of the value of writer_version.

[krvikash]

Edit: Validation of the writer_version should only be performed if the cdf is enabled.

[krvikash]

can we verify writer_version as well here along with change_data_feed_enabled?

[krvikash]

LGTM. few comments.

[krvikash]

We can remove this. Already covered inside testCreateTableWithCdfPropertyWriterVersionNotUpgraded.

[krvikash]

we can include test for ALTER TABLE " + tableName + " SET PROPERTIES change_data_feed_enabled = false, writer_version = 3

[krvikash]

nit: tableName1 -> tableName

[alexjo2144]

Suggested change

	metadataEntry = Optional.of(buildMetadataEntry(session, handle, configuration, createdTime));
	if (changeDataFeedEnabled && ((writerVersion.isPresent() && writerVersion.get() < CDF_SUPPORTED_WRITER_VERSION) ||
	(currentProtocolEntry.getMinWriterVersion() < CDF_SUPPORTED_WRITER_VERSION))) {
	metadataEntry = Optional.of(buildMetadataEntry(session, handle, configuration, createdTime));
	long newWriterVersion = writerVersion.orElse(currentProtocolEntry.getMinWriterVersion());
	if (changeDataFeedEnabled && newWriterVersion < CDF_SUPPORTED_WRITER_VERSION) {

[alexjo2144]

Move up with the other public properties

[ebyhr]

/test-with-secrets sha=37f9211dfa8fc250637eee45015afcd811ce5e90

[github-actions]

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/4288682058

[ebyhr]

/test-with-secrets sha=ba0dfb55a6ac1b1601f113810b7590bfe5064362

We're going to increase writer version automatically.

[krvikash]

double space.

[homar]

closing in favour of #16310