[Verifier] Support reuse of source query output table by gggrace14 · Pull Request #22169 · prestodb/presto

gggrace14 · 2024-03-12T16:22:31Z

Add support to Presto Verifier to reuse the output tables of the source query for control and test. Add configs --control-reuse-table and --test-reuse-table to turn the feature on and off. If reuse-table is turned on, skip the execution of main query as well as the setup and teardown queries. Compute checksums on the output table of main query directly.

== NO RELEASE NOTE ==

mbasmanova · 2024-03-12T21:12:03Z

@gggrace14 Ge, how much faster verifier runs with this optimization?

spershin · 2024-03-12T23:43:16Z

@gggrace14 Ge, how much faster verifier runs with this optimization?

@mbasmanova

This needs to be thoroughly tested to get some numbers and also depends on workflow and number of queries.
We can run this for a while in Native and compare the difference.

But, obviously it is the time without optimization minus time it takes to run setup, main and teardown queries.
Note, that it looks like (I noticed) the verifier runs control query 1st and then runs the test one, so both main queries are SERIALIZED.
Not sure if there is a verifier option to run them in parallel.

gggrace14 · 2024-03-12T23:58:02Z

class QueryRewriter, class AbstractVerification and class VerifierDao have the main changes. Others are about passing the required info around.

gggrace14 · 2024-03-13T00:02:26Z

Not sure if there is a verifier option to run them in parallel.

For control & test checksum queries, current implementation is to run them in sequence. For main queries, param --concurrent-control-and-test can run them in parallel.

spershin

@gggrace14
Thanks for this change (and others) to enabled re-use of the tables and reduce verification time and compute.

The change looks good to me, few questions.

Maybe worth @kewang1024, @ajaygeorge or @aweisberg to take a quick look at it too.

spershin · 2024-03-12T23:56:25Z

presto-jdbc/pom.xml

Curious, why is this required in this PR?

This is the very tricky part of mvn dependencies that I need to check with java experts. No one knows about it so far. The above is the only one solution I find online.

In general, when packaging, this jdbc/pom.xml changes the path or namespace of presto.client

<relocation> <pattern>com.facebook.presto.client</pattern> <shadedPattern>${shadeBase}.client</shadedPattern> </relocation>

This causes compiling error for the following line called by my change
In presto-jdbc

import com.facebook.presto.client.StatementStats; ... public static QueryStats create(String queryId, StatementStats stats) {}

The above is the only one solution I find online. I will separate this pom change into a standalone PR, for people to find the change easily. Need to check what it means to the java build.

Figured out a way to avoid this attribute and changing the packaging.

spershin · 2024-03-13T00:02:14Z

presto-verifier/src/main/java/com/facebook/presto/verifier/source/VerifierDao.java

Strange, didn't we put control and test query Ids into the table before this change?
The ones that you click on, like "Control Query ID".

Yeah, the "Control Query ID" link on UI is populated with the query_id returned by the actual control query run triggered by Verifier today. If we reuse the shadow/prod query output and don't trigger the control/test query runs, we have to pass the query_ids all the way to Verifer through xdb test suite table, so that the UI can display them.

Otherwise the debugging experience is bad. Found it out from testing.

spershin · 2024-03-13T00:05:25Z

presto-verifier/pom.xml

What do we need from the presto-client in the verifier?

One place is the following lines of change. The change is about passing the query_id from xdb to the UI by setting them in the related data structures.

In presto-verifier

import com.facebook.presto.client.StatementStats; ... public static QueryActionStats queryIdStats(String queryId) { return new QueryActionStats(Optional.of(QueryStats.create(queryId, StatementStats.builder().setState(QueryState.FINISHED.toString()).build())), Optional.empty()); }

kewang1024 · 2024-03-14T00:20:07Z

presto-verifier/src/main/java/com/facebook/presto/verifier/framework/AbstractVerification.java

+                controlReuseTable = control.isPresent() && control.get() instanceof QueryObjectBundle && ((QueryObjectBundle) control.get()).isReuseTable();
+                if (controlReuseTable) {
+                    controlQueryContext.setState(QueryState.REUSE);
+                    controlQueryContext.setMainQueryStats(QueryActionStats.queryIdStats(sourceQuery.getQueryId(CONTROL).get()));
+                }


It would be clearer to put the logic inside the getQueryRewrite()

kewang1024 · 2024-03-14T00:20:16Z

presto-verifier/src/main/java/com/facebook/presto/verifier/framework/AbstractVerification.java

+            testReuseTable = test.isPresent() && test.get() instanceof QueryObjectBundle && ((QueryObjectBundle) test.get()).isReuseTable();
+            if (testReuseTable) {
+                testQueryContext.setState(QueryState.REUSE);
+                testQueryContext.setMainQueryStats(QueryActionStats.queryIdStats(sourceQuery.getQueryId(TEST).get()));
+            }


same as above

kewang1024 · 2024-03-14T00:24:07Z

presto-verifier/src/main/java/com/facebook/presto/verifier/rewrite/QueryRewriteConfig.java

+        return reuseTable;
+    }
+
+    @ConfigDescription("If true, reuse the output table of the source query. Otherwise, run the query and write to a temporary shadow table.")


1.Is it the best effort?
if it is set to true, but the output table is deleted, does the verification fail or follow original routine?

2.Does it only apply to INSERT and CTAS queries? if so, we should specify in the config description

If the output table is deleted, the verification will fail with the table not exists error. The user is responsible to ensure the output table exists.

Revised.

steveburnett · 2024-04-26T18:27:59Z

Suggest adding

documentation for the new configuration properties.

A good location might be near the control.table-prefix and test.table-prefix entries (lines 275-276) in https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/admin/verifier.rst?plain=1.
a release note entry. Perhaps something like the following:

== RELEASE NOTES ==

Verifier Changes
* Add  `control-reuse-table` and `test-reuse-table` configuration properties for the Presto Verifier to reuse the output tables of the source query for control and test :pr:`22169`

presto-verifier/src/main/java/com/facebook/presto/verifier/event/QueryInfo.java

arhimondr · 2024-05-09T14:43:02Z

presto-verifier/src/main/java/com/facebook/presto/verifier/framework/VerificationManager.java

                Optional<VerifierQueryEvent> event = result.getEvent();
                completed++;
-                if (!event.isPresent()) {
-                    statusCount.compute(SKIPPED, (status, count) -> count == null ? 1 : count + 1);


Why is this no longer necessary?

!event.isPresent() is when the test case is to be resubmitted. From our experience, we find it confusing if we also add the resubmission count to the counter SKIPPED. This problem becomes more apparent when reuse-table is enabled, as we resubmit more to be safe.

The main condition of SKIPPED (line 361) is when the control query run fails, ex., due to syntax error, which is how ppl usually assume the counter SKIPPED. When SKIPPED includes resubmissions as today, we find it confusing that the counters SUCCEED, FAILED and SKIPPED don't add up to the total test cases, as one test case can be resubmitted multiple times. So I split it in this change and add one more counter RESUBMITTED (line 366).

...rifier/src/main/java/com/facebook/presto/verifier/source/PrestoQuerySourceQuerySupplier.java

presto-verifier/src/test/java/com/facebook/presto/verifier/rewrite/TestQueryRewriter.java

arhimondr · 2024-05-09T15:16:25Z

presto-verifier/src/main/java/com/facebook/presto/verifier/rewrite/QueryRewriter.java

            QualifiedName originalTableName = insert.getTarget();
+            if (shouldReuseTable) {
+                return new QueryObjectBundle(
+                        originalTableName,


What if the original table contains some other data then the data inserted by the query being verifier. How do we know what partitions of the original table to compute the checksum over?

A good question! I have a next PR on top of this one that assembles a partition predicate and applies it to the checksum query. That needs a separate PR. #22965

presto-verifier/src/main/java/com/facebook/presto/verifier/framework/AbstractVerification.java

arhimondr · 2024-05-09T15:19:57Z

presto-verifier/src/main/java/com/facebook/presto/verifier/framework/AbstractVerification.java


    private final QueryActions queryActions;
-    private final SourceQuery sourceQuery;
+    private SourceQuery sourceQuery;


nit: consider keeping it final and return a new instance in VerificationResult (instead of this)

A good catch. Also thought it's better if I can make this class immutable. However, I can't figure out how the constructor can look like, esp. since this class AbstractVerification is an abstract class. Please let me know if you have a good suggestion.

I thought I could add an abstract method to this class

public abstract AbstractVerification withSourceQuery(SourceQuery sourceQuery);

Then its subclass DataVerification overrides this method, creates and returns a DataVerification instance. However, we can't access those private fields, ex. queryActions, of the base abstract class in the subclass when calling the DataVerification constructor.

Hi @arhimondr , I figured out a different way that doesn't need to make this sourceQuery field mutable. The original objective is to pass a signal so that the "resubmitted" verification workflow will not reuse output tables. Previously I constructed a new SourceQuery instance. A different way I figured is to check the VerificationContext.getResubmissionCount() as below. I figured I'm able to access VerificationContext when calling queryRewriter.rewriteQuery()

@Override protected QueryObjectBundle getQueryRewrite(ClusterType clusterType) { return queryRewriter.rewriteQuery(getSourceQuery().getQuery(clusterType), getSourceQuery().getQueryConfiguration(clusterType), clusterType, getVerificationContext().getResubmissionCount() == 0); }

gggrace14 · 2024-06-09T05:20:57Z

Suggest adding

documentation for the new configuration properties.
A good location might be near the control.table-prefix and test.table-prefix entries (lines 275-276) in https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/admin/verifier.rst?plain=1.

a release note entry. Perhaps something like the following:
== RELEASE NOTES ==

Verifier Changes
* Add  `control-reuse-table` and `test-reuse-table` configuration properties for the Presto Verifier to reuse the output tables of the source query for control and test :pr:`22169`

Adding the corresponding documentation to the verifier.rst.

For the release note, I have a next PR #22965 on top of this for the complete feature. I'd prefer to add release note when the complete feature is in, to expose it to the user.

steveburnett

Thanks for the doc! Nit of formatting, no other concerns.

steveburnett · 2024-06-10T19:11:59Z

presto-docs/src/main/sphinx/admin/verifier.rst

                                            are emitted to ``stdout``.
 ``control.table-prefix``                    The table prefix to be appended to the control target table.
 ``test.table-prefix``                       The table prefix to be appended to the test target table.
+``control.reuse-table``                     If true, reuse the output table of the control source Insert and CreateTableAsSelect


Suggested change

``control.reuse-table`` If true, reuse the output table of the control source Insert and CreateTableAsSelect

``control.reuse-table`` If ``true``, reuse the output table of the control source Insert and CreateTableAsSelect

Nit for formatting, same as #22965.

steveburnett · 2024-06-10T19:12:11Z

presto-docs/src/main/sphinx/admin/verifier.rst

 ``test.table-prefix``                       The table prefix to be appended to the test target table.
+``control.reuse-table``                     If true, reuse the output table of the control source Insert and CreateTableAsSelect
+                                            query. Otherwise, run the control source query and write to a temporary table.
+``test.reuse-table``                        If true, reuse the output table of the test source Insert and CreateTableAsSelect


Suggested change

``test.reuse-table`` If true, reuse the output table of the test source Insert and CreateTableAsSelect

``test.reuse-table`` If ``true``, reuse the output table of the test source Insert and CreateTableAsSelect

Add support to Presto Verifier to reuse the output tables of the source query for control and test. Add configs --control-reuse-table and --test-reuse-table to turn the feature on and off. If reuse-table is turned on, skip the execution of main query as well as the setup and teardown queries. Compute checksums on the output table of main query directly.

gggrace14 · 2024-06-13T00:21:47Z

Thanks for the doc! Nit of formatting, no other concerns.

Hi @steveburnett , revised the format of verifier.rst document. Would you be able to help review again?

gggrace14 · 2024-06-13T14:55:50Z

Thanks for the doc! Nit of formatting, no other concerns.

Hi @steveburnett , revised the format of verifier.rst document. Would you be able to help review again?

Hi @steveburnett , thanks for the review! It looks to need your approval to be able to merge.

steveburnett

LGTM! (docs)

Pull updated branch, new local build, looks good. Thanks!

steveburnett · 2024-06-13T15:12:24Z

Not a required test so not blocking merge, but the test

singlestore tests / singlestore-dockerized-tests (pull_request)

failed here. I believe I've seen other mentions of that test being flaky lately, wanted to mention it in case people are looking for examples of it failing.

gggrace14 · 2024-06-13T18:29:20Z

LGTM! (docs)

Pull updated branch, new local build, looks good. Thanks!

Thank you, @steveburnett !

gggrace14 requested review from a team as code owners March 12, 2024 16:22

gggrace14 requested review from ericyuliu, presto-oss and spershin March 12, 2024 16:22

gggrace14 force-pushed the scaledverifierbase branch 2 times, most recently from b9385cd to 6746754 Compare March 12, 2024 20:33

spershin previously approved these changes Mar 13, 2024

View reviewed changes

spershin requested review from ajaygeorge and kewang1024 March 13, 2024 00:33

gggrace14 dismissed spershin’s stale review via 5ba5459 March 13, 2024 17:38

gggrace14 force-pushed the scaledverifierbase branch 3 times, most recently from ea03b3b to df80863 Compare March 13, 2024 22:09

kewang1024 reviewed Mar 14, 2024

View reviewed changes

gggrace14 force-pushed the scaledverifierbase branch 2 times, most recently from 9f54569 to e1c2eb6 Compare March 21, 2024 04:44

gggrace14 force-pushed the scaledverifierbase branch 2 times, most recently from ce525d2 to b41a863 Compare April 3, 2024 21:45

gggrace14 force-pushed the scaledverifierbase branch from b41a863 to cd55682 Compare April 9, 2024 17:21

gggrace14 force-pushed the scaledverifierbase branch from cd55682 to d1c9520 Compare April 26, 2024 15:58

arhimondr reviewed May 9, 2024

View reviewed changes

gggrace14 force-pushed the scaledverifierbase branch from d1c9520 to eef0472 Compare June 7, 2024 14:35

gggrace14 force-pushed the scaledverifierbase branch from eef0472 to ece9d5e Compare June 9, 2024 19:02

gggrace14 requested a review from steveburnett as a code owner June 9, 2024 19:02

gggrace14 requested a review from arhimondr June 10, 2024 15:43

steveburnett requested changes Jun 10, 2024

View reviewed changes

gggrace14 force-pushed the scaledverifierbase branch 2 times, most recently from 3f915f8 to 7f4ee12 Compare June 12, 2024 21:45

gggrace14 force-pushed the scaledverifierbase branch from 7f4ee12 to 417472a Compare June 12, 2024 23:34

arhimondr approved these changes Jun 13, 2024

View reviewed changes

gggrace14 requested a review from steveburnett June 13, 2024 00:19

steveburnett approved these changes Jun 13, 2024

View reviewed changes

gggrace14 merged commit 2ef2ba8 into prestodb:master Jun 13, 2024

gggrace14 deleted the scaledverifierbase branch June 13, 2024 18:29

	``control.reuse-table`` If true, reuse the output table of the control source Insert and CreateTableAsSelect
	``control.reuse-table`` If ``true``, reuse the output table of the control source Insert and CreateTableAsSelect

	``test.reuse-table`` If true, reuse the output table of the test source Insert and CreateTableAsSelect
	``test.reuse-table`` If ``true``, reuse the output table of the test source Insert and CreateTableAsSelect

Conversation

gggrace14 commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbasmanova commented Mar 12, 2024

Uh oh!

spershin commented Mar 12, 2024

Uh oh!

gggrace14 commented Mar 12, 2024

Uh oh!

gggrace14 commented Mar 13, 2024

Uh oh!

spershin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gggrace14 Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gggrace14 Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gggrace14 Jun 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveburnett commented Apr 26, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gggrace14 Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gggrace14 Jun 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gggrace14 commented Jun 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gggrace14 commented Mar 12, 2024 •

edited

Loading

gggrace14 Mar 13, 2024 •

edited

Loading

gggrace14 Mar 13, 2024 •

edited

Loading

gggrace14 Jun 9, 2024 •

edited

Loading

gggrace14 Jun 6, 2024 •

edited

Loading

gggrace14 Jun 13, 2024 •

edited

Loading

gggrace14 commented Jun 9, 2024 •

edited

Loading