add new field updatedRows to QueryStatistics.java where it's availabl…

[bhzaeri]

…e in EventListener callbacks. It comes from TableMutationOperator.java where update/delete queries are issued.

add method recordUpdatedPositions to the OperatorContext class update the updatedPositions for the queries run through MergeWriterOperator.java

Description

The issue that this pull request tries to fix is that after running UPDATE/DELETE queries, the outputRows in QueryStatistics.java is always 1. But we need the actual value of updated rows. We figured out that the number of updated rows is returned in TableMutationOperator.java method getOutput() and also in MergeWriterOperator.java the same method. The updated rows number is passed to operatorContext instance and from there, all the way down to the QueryStatistics.java which make the number available in the event listeners.
So far, we have tested this successfully on SQL Serevr, Hive, Mysql, and Postgresql.

Additional context and related issues

In our ransomware defender platform, we need to monitor the behavior of users who have access to run queries on the DBs connected via Trino. So, we need to be notified of users' actions and the exact results of their actions. Trino returns the correct values for SELECT and INSERT queries. We need the same for UPDATE/DELTE as well.

The only issue is that the current pull request doesn't include the proper unit tests for the changes. Could you please help us with the proper way to write the unit tests?

Release notes

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

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

#24596

[cla-bot]

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

[bhzaeri]

Hi @chenjian2664 @wendigo
I added updating of the rowCount in the MergeWriterOperator.java as well as TableMutationOperator.java, as you advised in the last pull request. I tested the current branch successfully with Hive, SQLServer, Mysql, and Postgresql.
Could you please take a look when you get the chance?
Bahram

[cla-bot]

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

[chenjian2664]

Is it possible to add a test for it?
Please rephrase commit https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md

[bhzaeri]

That's actually my next question.
Is there a similar unit test to take a look and get the idea on how to implement the unit test for this?
Thanks!

[bhzaeri]

Hi @chenjian2664 @wendigo
I have added unit tests for the updatd rows. Since the connector in the event listeners test did not support update/delete, I had to add the new tests for update/delete to the BaseConnectorTest, inherited by all the connector tests for different DBs.
Please take a look and let me know if there are any issues.
Thanks!
Bahram

[chenjian2664]

cc @Praveen2112
For the EventListener, I think you may have some thoughts

[chenjian2664]

There already exists a EventsCollector, why not reuse it?

[bhzaeri]

I explained it in the previous commit message:
The test (for UPDATE/DELETE queries) was added to BaseConnectorTest.java and it needs the classes like EventsAwaitingQueries and QueryEvents that exist in trino-tests module. So, I moved the needed classes from trino-tests to trino-testing where they are exposed to both trino-tests and trino-testing.
I removed those classes from trino-tests and added to trino-testing so both the old and the new tests can have access to them.

[chenjian2664]

Same, why not reuse it

[bhzaeri]

Please refer to the last review.

[chenjian2664]

For the commits head wrap to 50 characters, body to 72 characters https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md.

[bhzaeri]

Hi @Praveen2112 @chenjian2664 @wendigo
Any thoughts?