Make rowid optional in DELETE query plan

[ghelmling]

Summary: Currently, the $row_id column is automatically inserted into the output variables for DELETE queries by QueryPlanner. If a connector does not actually use $row_id to implement DELETE, then we should not require it. This makes $row_id optional. If the Optional is empty, then we don't need to project the output variable.

Differential Revision: D76325048

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D76325048

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D76325048

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D76325048

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D76325048

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D76325048

[hantangwangd]

One little question: should we consider backward compatibility for potential externally-implemented connectors?

[ghelmling]

Do you have a preference for how you would want to support this instead?

I suppose another option would be to add a guard method to the interface that could disable the behavior:
boolean requiresDeleteRowIdColumnHandle()

That seems kind of cumbersome to have 2 methods for both the update and delete rowId handling. I thought that the Optional usage would encapsulate this better.

Or we could add a system session property that QueryPlanner checks before injecting the rowId. That would avoid the ConnectorMetadata API change. But that doesn't seem quite as clean, as whether or not the rowId column is required is more a function/requirement of the connector implementation. So you could theoretically be using different connectors with different requirements.

I'm open to any other suggestions.

[hantangwangd]

If we need to ensure backward compatibility for this Connector SPI change, the modification should not affect any potentially existing external connectors that may have already implemented the modified SPI methods getDeleteRowIdColumnHandle or getUpdateRowIdColumnHandle.

IMO, we can refer to a previous SPI method modification that took backward compatibility into account, see here. In this approach, we retain the original SPI methods but mark them as @deprecated, while adding new SPI methods with default implementations that internally call the legacy methods. Meanwhile, the engine's internal implementation logic now exclusively uses new SPI methods. This ensures that the SPI changes remain backward-compatible, leaving all existing connectors unaffected.

One minor issue with this approach is that the methods need to maintain distinct signatures, which may require using different method names for the new ones. The code would roughly look like this:

    @Deprecated
    default ColumnHandle getUpdateRowIdColumnHandle(ConnectorSession session, ConnectorTableHandle tableHandle, List<ColumnHandle> updatedColumns)
    {
        throw new PrestoException(NOT_SUPPORTED, "This connector does not support updates");
    }

    default Optional<ColumnHandle> getUpdateRowIdColumn(ConnectorSession session, ConnectorTableHandle tableHandle, List<ColumnHandle> updatedColumns)
    {
        ColumnHandle columnHandle = getUpdateRowIdColumnHandle(session, tableHandle, updatedColumns);
        return Optional.ofNullable(columnHandle);
    }

In this way, all existing connectors (especially external-implemented connectors which we can not modify directly) would not be affected by this SPI interface change. And we can remove these deprecated SPI methods in some major version upgrade that breaks backward compatibility.

What's your opinion about this?

[ghelmling]

Thanks for the reply. Yeah, that approach makes sense. I can make that update.

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D76325048

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D76325048

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D76325048

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D76325048

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D76325048

[ghelmling]

@hantangwangd would you mind taking another look? The FB internal failing test is due to a corresponding internal code change needed.

[hantangwangd]

Thanks for the refactoring, I have verified it on Iceberg V1 tables that only support metadata deletion. Looks good to me, just some little nits.

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D76325048