Disable ALTER TABLE SET PROPERTIES task

[ebyhr]

Disable until we fix some issues (nested properties, reset values to their default, etc).

[findepi]

The shortcomings do not warrant the statement to be disabled. We should improve the shortcomings instead of killing the feature.

[martint]

@findepi, we want to do a release in the next day or so. If we don't have enough time to finish the implementation by then, we should disable it for now. We don't want to ship something we'll have to break in the next release.

[findepi]

Per my understanding on the discussion among maintainers, the shortcomings are going to be addressed by incremental improvement of the current syntax, so we don't need to disable anything for interim. @martint Please help me understand, if i got the conversation wrong.

[martint]

The changes from the current syntax are not incremental. Currently, we have:

ALTER TABLE SET PROPERTIES (x = 1, b = 2, ...)

The updated syntax is intended to match the syntax for UPDATE:

UPDATE t SET x = 1, y = 2, ....

(in the future, we may also want to match this form of UPDATE: UPDATE t SET (x, y) = (1, 2)

Thus, the updated syntax would be:

ALTER TABLE SET PROPERTIES x = 1, b = 2, ...

Additionally, the SPI is not appropriate for supporting setting nested properties or default values. Even if we're not planning on implementing that in the immediate future, we should update the SPI to allow for it. In its current shape, we'd need deprecate the API that's been implemented, have a transition path to the new one, etc. It seems pointless given that this is a new feature, so let's get that part right.

[findepi]

the SPI is not appropriate for supporting setting nested properties

i'd argue this is actually dangerous feature to have.
modifying a component of a property, may make it into an inconsistent state in case of concurrent writes.

collections are used eg to denote partitioning, and this should be modified as "a unit".

or default values

this should be handled by providing a list of properties to reset, as a separate parameter. (edit: see below)

we should update the SPI to allow for it. In its current shape, we'd need deprecate the API that's been implemented

I am sure you know this isn't actually a problem, especially for an API that was recently introduced and is not widely adopted.
compare this to evolution of ConnectorPageSourceProvider API, which is implemented by every connector:

$ git lg  core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorPageSourceProvider.java
6bb9bfc68f - Remove unecessary default implementation in ConnectorPageSourceProvider (3 months ago) <Raunaq Morarka>
16cf8aa070 - Remove deprecated APIs in ConnectorPageSourceProvider (5 months ago) <Raunaq Morarka>
03d8e5abc6 - Repackage as io.trino (step 4 of 5): import statements (10 months ago) <David Phillips>
cea58426e0 - Repackage as io.trino (step 3 of 5): package statements (10 months ago) <David Phillips>
30b6b68f6c - Repackage as io.trino (step 1 of 5): move files (10 months ago) <David Phillips>
ede1b5f759 - Deprecate ConnectorPageSourceProvider#createPageSource method (1 year, 2 months ago) <Karol Sobczak>
214c08fbad - Allow blocking page source until dynamic filters are ready (1 year, 2 months ago) <Roman Zeyde>
67368f6a03 - Run code formatter (1 year, 5 months ago) <David Phillips>
f6ad3e5cbd - Deprecate createPageSource overload (1 year, 7 months ago) <Piotr Findeisen>
cdede7ea07 - Implement local dynamic filtering for broadcast inner-joins (2 years, 1 month ago) <Roman Zeyde>
4da3b8870a - Remove deprecated API from ConnectorPageSourceProvider (2 years, 4 months ago) <praveenkrishna>
e2e6fd3326 - Pass TableHandle to ConnectorPageSourceProvider (2 years, 7 months ago) <David Phillips>

Currently, we have:

ALTER TABLE SET PROPERTIES (x = 1, b = 2, ...)

[...]
Thus, the updated syntax would be:

ALTER TABLE SET PROPERTIES x = 1, b = 2, ...

Both syntaxes will work, I am fine to have it your way and I agree the parens feel redundant.
I agree this is important, as it's user-facing and we should abstain from making user-facing changes after the feature is out of the door.
Would it be hard to make such a change?

[findepi]

or default values

this should be handled by providing a list of properties to reset, as a separate parameter.

Actually @ebyhr noticed this likely won't require SPI change.

[findepi]

Currently, we have:

ALTER TABLE SET PROPERTIES (x = 1, b = 2, ...)

[...]
Thus, the updated syntax would be:

ALTER TABLE SET PROPERTIES x = 1, b = 2, ...

-> #9813

🤷‍♂️