Return number of deleted rows from Iceberg metadata delete

[homar]

Description

Related issues, pull requests, and links

fixes #12055

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

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

[findepi]

When can the information be missing?

if it can actually be missing, we should return empty instead of 0,
but maybe it's guaranteed to exist and then we just should get and check?

[homar]

Are you sure? I actually think that it is better to return 0, it is more consistent for me when all deletes return information in the same way. Even tests show this as in current state this passes:
assertUpdate("DELETE FROM " + tableName + " WHERE key = 'three'", 0);
and it won't pass with the proposed change.
even this will fail:
assertQueryReturnsEmptyResult("DELETE FROM " + tableName + " WHERE key = 'three'");
with:
java.lang.AssertionError: [Rows for query [DELETE FROM test_delete_returns_number_of_rows_1npq8u144q WHERE key = 'three']] Expecting empty but was:<[[null]]>

@findepi WDYT?

[homar]

When can the information be missing?

When nothing is deleted this property is not set.

[findepi]

information in the same way. Even tests show this as in current state this passes:
assertUpdate("DELETE FROM " + tableName + " WHERE key = 'three'", 0);
and it won't pass with the proposed change.

use assertUpdate("DELETE..."); (remove , 0) there

When nothing is deleted this property is not set.

Sounds like something that we should be possible to fix.

Until it's fixed, i don't want to rely on a quirk that lack of DELETED_RECORDS_PROP means "0 rows affected".
That sounds like relying on an impl detail.

[alexjo2144]

Can you remove the two test overrides in BaseIcebergConnectorTest now? https://github.com/trinodb/trino/blob/master/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java#L242

[findepi]

can we make Iceberg fill DELETED_RECORDS_PROP always?
let's file a PR and link with a TODO note here.

[findepi]

can we make Iceberg fill DELETED_RECORDS_PROP always?
let's file a PR and link with a TODO note here.

[findepi]

thanks for adding a TODO note. Can it contain a link?

[homar]

I don't have a pr yet. Should I open an issue? What if they decide to close it ?

[findepi]

let's have an issue.

even if it gets closed, it will serve as an anchor for further information and current status

[findepi]

apache/iceberg#4647

[findepi]

CI #12151