IGNITE-22557 Create system view for SQL query plans history #11425

oleg-vlsk · 2024-07-10T00:32:10Z

Thank you for submitting the pull request to the Apache Ignite.

In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:

The Contribution Checklist

There is a single JIRA ticket related to the pull request.
The web-link to the pull request is attached to the JIRA ticket.
The JIRA ticket has the Patch Available state.
The pull request body describes changes that have been made.
The description explains WHAT and WHY was made instead of HOW.
The pull request title is treated as the final commit message.
The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
A reviewer has been mentioned through the JIRA comments
(see the Maintainers list)
The pull request has been checked by the Teamcity Bot and
the green visa attached to the JIRA ticket (see TC.Bot: Check PR)

Notes

If you need any help, please email [email protected] or ask anу advice on http://asf.slack.com #ignite channel.

alex-plekhanov · 2024-08-16T12:43:50Z

...es/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/dml/UpdatePlan.java

+            "; key column index=" + (keyColIdx < 0 ? "N/A" : keyColIdx) +
+            "; value column index=" + (valColIdx < 0 ? "N/A" : valColIdx) +
+            "; row count=" + rowsNum +
+            "; values=" + (rows == null ? "N/A" : "[" + rowsToString(rows) + "]") +


Not sure we should add rows to plan. Plan will be different each time and it will wash out all other plan. For me value of DML plans are not clear. Perhaps it will be useful to have only plan for selectQry, but looks like we don't have this plan during update. Perhaps these queries are tracked on map phase.

The concept of plans for DML commands has been completely changed.

alex-plekhanov · 2024-08-16T12:44:47Z

...c/main/java/org/apache/ignite/internal/processors/query/h2/twostep/GridMapQueryExecutor.java

@@ -472,6 +473,17 @@ private void onQueryRequest0(
                            );
                        }

+                        SqlPlanHistoryTracker planHistTracker = ctx.query().runningQueryManager().planHistoryTracker();


h2.runningQueryManager().planHistoryTracker().addPlan(...)?

alex-plekhanov · 2024-08-16T12:45:02Z

...ain/java/org/apache/ignite/internal/processors/query/h2/twostep/GridReduceQueryExecutor.java

@@ -522,6 +523,17 @@ else if (QueryUtils.wasCancelled(err))
                            );
                        }

+                        SqlPlanHistoryTracker planHistTracker = ctx.query().runningQueryManager().planHistoryTracker();


h2.runningQueryManager().planHistoryTracker().addPlan(...)?

alex-plekhanov · 2024-08-16T12:45:33Z

modules/core/src/main/java/org/apache/ignite/internal/processors/query/running/SqlPlanKey.java

+        this.schema = schema;
+        this.loc = loc;
+
+        hash = 31 * (31 * plan.hashCode() + qry.hashCode() + schema.hashCode()) + (loc ? 1 : 0);


Objects.hash(...)?

The Objects.hash() method has been implemented.

alex-plekhanov · 2024-08-16T12:46:36Z

modules/core/src/main/java/org/apache/ignite/internal/processors/query/running/SqlPlan.java

+    private final SqlPlanKey key;
+
+    /** */
+    private final SqlPlanValue val;


Why do we need extra SqlPlanValue class? Looks like everithing can be stored in SqlPlan

All SQL plan history data is now stored in the SqlPlan class; the SqlPlanKey and SqlPlanValue classes have been removed.

alex-plekhanov · 2024-08-16T12:51:14Z

...src/main/java/org/apache/ignite/internal/processors/query/running/SqlPlanHistoryTracker.java

+        if (historySize <= 0)
+            return;
+
+        SqlPlan sqlPlan = new SqlPlan(plan, qry, schema, loc, startTs, engine);


I think instead of three entities SqlPlan, SqlPlanKey, SqlPlanValue only one can be used (SqlPlan)
sqlPlanHistory can be just a map from SqlPlan to timestamp and we can add entries like:
sqlPlanHistory.put(sqlPlan.key(), U.currentTimeMillis());
We don't need extra parameter startTs since almost always it will be equals to U.currentTimeMillis().

Done (see previous comment).

alex-plekhanov · 2024-08-16T12:54:08Z

...src/main/java/org/apache/ignite/internal/processors/query/running/SqlPlanHistoryTracker.java

+    }
+
+    /** */
+    public enum SqlEngine {


The core module shouldn't know about list of query engines. Let's use String instead of this enum. Use IndexingQueryEngineConfiguration.ENGINE_NAME and CalciteQueryEngineConfiguration.ENGINE_NAME from indexing and calcite modules.

alex-plekhanov · 2024-09-24T15:23:14Z

...es/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/dml/UpdatePlan.java

@@ -80,10 +80,10 @@ public final class UpdatePlan {

    /** Number of rows in rows based MERGE or INSERT. */
    private final int rowsNum;
-


Redundant changes (all in this file)

alex-plekhanov · 2024-09-24T15:25:02Z

...src/main/java/org/apache/ignite/internal/processors/query/running/SqlPlanHistoryTracker.java

+
+        SqlPlan sqlPlan = new SqlPlan(plan, qry, schema, loc, engine);
+
+        sqlPlanHistory.put(sqlPlan, sqlPlan.startTime());


Are you sure key will be updated after this call.
You use information only from key (including startTime) in the view, if key is not updated, than view will contain a wrong startTime field value.

Yes, the key wasn't updated during the put() operation. To fix it, I changed the SQL plan history data structure from GridBoundedConcurrentLinkedHashMap to GridBoundedConcurrentLinkedHashSet and edited the addPlan() method, so that newer entries would replace older ones with the same SQL plan.

alex-plekhanov · 2024-09-25T14:46:31Z

...src/main/java/org/apache/ignite/internal/processors/query/running/SqlPlanHistoryTracker.java

+     * @param historySize History size.
+     */
+    public void setHistorySize(int historySize) {
+        this.historySize = historySize;


Let's recreate sqlPlanHistory here. Now this method used only once in test with value 0, but if someone will use this method (perhaps in test too) with another value, result can be unpredicted.

alex-plekhanov · 2024-09-25T14:49:26Z

...gnite/internal/processors/query/calcite/integration/SqlPlanHistoryConfigIntegrationTest.java

+import org.junit.Test;
+
+/** Test for Sql plan history configuration. */
+public class SqlPlanHistoryConfigIntegrationTest extends GridCommonAbstractTest {


Let's move this test to ignite-spring module, instead of adding new dependency to ignite-calcite.

SqlPlanHistoryConfigIntegrationTest has been moved to the ignite-spring module.

alex-plekhanov · 2024-09-25T14:58:23Z

...ache/ignite/internal/processors/query/calcite/integration/SqlPlanHistoryIntegrationTest.java

+     * @return Cache configuration.
+     */
+    @SuppressWarnings("unchecked")
+    private CacheConfiguration configureCahce(String name, Class<?>... idxTypes) {


Typo: Cahce

alex-plekhanov · 2024-09-25T15:01:48Z

...ache/ignite/internal/processors/query/calcite/integration/SqlPlanHistoryIntegrationTest.java

+
+/** Tests for SQL plan history (Calcite engine). */
+@RunWith(Parameterized.class)
+public class SqlPlanHistoryIntegrationTest extends GridCommonAbstractTest {


Test already Parameterized. Perhaps it's worth to add parameters queryEngine and clientMode instead of creating child classes?

queryEngine and clientMode have been added to the list of parameters. Test subclasses for H2 and clients have been removed.

alex-plekhanov · 2024-09-25T15:04:45Z

...ache/ignite/internal/processors/query/calcite/integration/SqlPlanHistoryIntegrationTest.java

+        queryNode().context().query().runningQueryManager().resetPlanHistoryMetrics();
+
+        mapNode().context().query().runningQueryManager().resetPlanHistoryMetrics();


for (IgniteEx ignite: G.allGrids()) ignite.context().query().runningQueryManager().resetPlanHistoryMetrics();

alex-plekhanov · 2024-09-25T15:07:16Z

...ache/ignite/internal/processors/query/calcite/integration/SqlPlanHistoryIntegrationTest.java

+    /** Checks successful JDBC queries. */
+    @Test
+    public void testJdbcQuery() throws SQLException {
+        if (loc)


To ignore test it's better to use assumeTrue/assumeFalse statements. In your case assumeFalse(loc).
Tests will be marked as ignored instead of passed (and we can also provide a reason, why they are ignored).

SqlPlanHistoryIntegrationTest has been refactored using the assumeFalse statements.

alex-plekhanov · 2024-09-25T15:13:02Z

Please resolve also merge conflicts

alex-plekhanov · 2024-10-04T06:29:19Z

modules/spring/src/test/java/org/apache/ignite/testsuites/IgniteSpringTestSuite.java

-    CdcConfigurationTest.class
+    CdcConfigurationTest.class,
+
+    SqlPlanHistoryConfigTest.class


Let's add comma at the end

Comma added.

…indexing

…tyPlanHistory added, minor Javadoc changes

…g#executeUpdate0 to IgniteH2Indexing#executeUpdate

…dConcurrentLinkedHashSet, SqlPlanHistoryTracker#addPlan changed, SqlPlanHistoryIntegrationTest#testEntryReplacement added

This reverts commit fb2a47c.

…HistoryIntegrationTest refactored with assumeFalse(), SqlPlanHistoryTracker#setHistorySize edited

… to GridBoundedConcurrentLinkedHashMap

…toryIntegrationTest

…ClientIntegrationTest, SqlPlanHistoryH2IntegrationTest and SqlPlanHistoryH2ClientIntegrationTest deleted

…to ignite-22557 Merge commit

alex-plekhanov reviewed Aug 16, 2024

View reviewed changes

alex-plekhanov reviewed Sep 24, 2024

View reviewed changes

alex-plekhanov reviewed Sep 25, 2024

View reviewed changes

alex-plekhanov reviewed Oct 4, 2024

View reviewed changes

oleg-vlsk and others added 25 commits October 8, 2024 07:17

IGNITE-22557 Tests added

8652ad7

IGNITE-22557 SQL Plan History view introduced

7485803

IGNITE-22557 SQL Plan History Configuration Test removed from ignite-…

2e4f27c

…indexing

IGNITE-22557 Tests added to IgniteCalciteTestSuite

95ab41b

IGNITE-22557 Tests minor refactoring

318ffdf

IGNITE-22557 Minor refactoring

bfd9fe4

IGNITE-22557 Minor refactoring

21b5807

IGNITE-22557 Minor refactoring

40f2291

IGNITE-22557 Minor UpdatePlan#plan refactoring

adeefb5

IGNITE-22557 Sequence of columns in the view changed

63831eb

IGNITE-22557 SqlPlanHistoryH2SelfTest refactoring

91833e4

IGNITE-22557 Description of the view added to system-views.adoc

27f46ab

IGNITE-22557 Tests refactoring, SqlPlanHistoryCalciteSelfTest#testEmp…

a338ccd

…tyPlanHistory added, minor Javadoc changes

IGNITE-22557 Adding plans for DML commands moved from IgniteH2Indexin…

ed7380c

…g#executeUpdate0 to IgniteH2Indexing#executeUpdate

IGNITE-22557 Changes made according to code review

9cb98f9

IGNITE-22557 Minor tests refactoring

febb24f

IGNITE-22557 GridBoundedConcurrentLinkedHashMap changed to GridBounde…

1667294

…dConcurrentLinkedHashSet, SqlPlanHistoryTracker#addPlan changed, SqlPlanHistoryIntegrationTest#testEntryReplacement added

IGNITE-22557 Whitespaces refactoring

8a437a2

IGNITE-22557 Whitespaces refactoring

886c763

Delete changes in UpdatePlan

657b86b

Revert "Delete changes in UpdatePlan"

be4551a

This reverts commit fb2a47c.

IGNITE-22557 Whitespaces refactoring

2177847

IGNITE-22557 Whitespaces refactoring

6bb9e06

IGNITE-22557 SqlPlanHistoryConfigTest moved to ignite-spring, SqlPlan…

776339b

…HistoryIntegrationTest refactored with assumeFalse(), SqlPlanHistoryTracker#setHistorySize edited

IGNITE-22557 GridBoundedConcurrentLinkedHashSet has been changed back…

5fa16ac

… to GridBoundedConcurrentLinkedHashMap

21518201 added 6 commits October 8, 2024 07:41

IGNITE-22557 sqlEngine and isClient added as parameters to SqlPlanHis…

e6eeaeb

…toryIntegrationTest

IGNITE-22557 other options for SqlPlanHistoryIntegrationTest#params

9a68a10

IGNITE-22557 SqlPlanHistoryIntegrationTest refactored, SqlPlanHistory…

31faf0c

…ClientIntegrationTest, SqlPlanHistoryH2IntegrationTest and SqlPlanHistoryH2ClientIntegrationTest deleted

IGNITE-22557 Javadoc for SqlPlanHistoryIntegrationTest corrected

2e6fdbe

IGNITE-22557 SqlPlanHistoryIntegrationTest refactored

850f43c

IGNITE-22557 Changes from master merged

0454592

oleg-vlsk force-pushed the ignite-22557 branch from 2d33e65 to 0454592 Compare October 7, 2024 23:15

21518201 and others added 3 commits October 8, 2024 14:07

IGNITE-22557 Changes for failed tests

183af7f

IGNITE-22557 SqlPlanHistoryIntegrationTest refactored

6ab7de6

Merge branch 'ignite-22557' of https://github.com/oleg-vlsk/ignite in…

9dd2cfc

…to ignite-22557 Merge commit

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IGNITE-22557 Create system view for SQL query plans history #11425

IGNITE-22557 Create system view for SQL query plans history #11425

oleg-vlsk commented Jul 10, 2024

alex-plekhanov Aug 16, 2024

oleg-vlsk Sep 10, 2024

alex-plekhanov Aug 16, 2024

oleg-vlsk Sep 10, 2024

alex-plekhanov Aug 16, 2024

oleg-vlsk Sep 10, 2024

alex-plekhanov Aug 16, 2024

oleg-vlsk Sep 10, 2024

alex-plekhanov Aug 16, 2024

oleg-vlsk Sep 10, 2024

alex-plekhanov Aug 16, 2024

oleg-vlsk Sep 10, 2024

alex-plekhanov Aug 16, 2024

oleg-vlsk Sep 10, 2024

alex-plekhanov Sep 24, 2024

oleg-vlsk Sep 25, 2024

alex-plekhanov Sep 24, 2024

oleg-vlsk Sep 25, 2024 •

edited

Loading

alex-plekhanov Sep 25, 2024

oleg-vlsk Oct 3, 2024

alex-plekhanov Sep 25, 2024

oleg-vlsk Oct 3, 2024

alex-plekhanov Sep 25, 2024

oleg-vlsk Oct 3, 2024

alex-plekhanov Sep 25, 2024

oleg-vlsk Oct 3, 2024

alex-plekhanov Sep 25, 2024

oleg-vlsk Oct 3, 2024

alex-plekhanov Sep 25, 2024

oleg-vlsk Oct 3, 2024

alex-plekhanov commented Sep 25, 2024

alex-plekhanov Oct 4, 2024

oleg-vlsk Oct 7, 2024

		@@ -80,10 +80,10 @@ public final class UpdatePlan {

		/** Number of rows in rows based MERGE or INSERT. */
		private final int rowsNum;


		SqlPlan sqlPlan = new SqlPlan(plan, qry, schema, loc, engine);

		sqlPlanHistory.put(sqlPlan, sqlPlan.startTime());

		queryNode().context().query().runningQueryManager().resetPlanHistoryMetrics();

		mapNode().context().query().runningQueryManager().resetPlanHistoryMetrics();

IGNITE-22557 Create system view for SQL query plans history #11425

Are you sure you want to change the base?

IGNITE-22557 Create system view for SQL query plans history #11425

Conversation

oleg-vlsk commented Jul 10, 2024

The Contribution Checklist

Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-vlsk Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-plekhanov commented Sep 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-vlsk Sep 25, 2024 •

edited

Loading