IGNITE-22717 SQL Calcite: User defined SQL views #11467

alex-plekhanov · 2024-08-07T15:51:57Z

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.

docs/_docs/sql-reference/ddl.adoc

Vladsz83 · 2024-08-12T14:43:17Z

...c/main/java/org/apache/ignite/internal/processors/query/calcite/schema/SchemaHolderImpl.java

-        igniteSchemas.forEach(newCalciteSchema::add);
+        newCalciteSchema.add(QueryUtils.DFLT_SCHEMA, new IgniteSchema(QueryUtils.DFLT_SCHEMA));
+
+        for (IgniteSchema schema : igniteSchemas.values())


I like similar to the previous: igniteSchemas.values().forEach(sch->sch.register(newCalciteSchema));

It's a matter of taste. sch->sch.register(newCalciteSchema) creates one additional object on each call for lambda.

...c/main/java/org/apache/ignite/internal/processors/query/calcite/schema/SchemaHolderImpl.java

...e/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/PlannerTest.java

...ain/java/org/apache/ignite/internal/processors/query/schema/management/SchemaDescriptor.java

Vladsz83 · 2024-08-13T10:40:05Z

...ain/java/org/apache/ignite/internal/processors/query/schema/management/SchemaDescriptor.java

+    }
+
+    /**
+     * Drop view.


Same: descriptor?

We drop view by descriptor.

Vladsz83 · 2024-08-13T11:43:43Z

...a/org/apache/ignite/internal/processors/query/stat/IgniteStatisticsConfigurationManager.java

-                    new StatisticsTarget(schemaName, typeDesc.tableName(), cols.toArray(EMPTY_STRINGS))
-                ),
-                false
+            mgmtBusyExecutor.execute(() ->


Can a related statistic survive for long if the executor is busy for long by an active statistics collection tasks?

I think it's not so critical. If column doesn't exists, statistics for this column will no hurt anyone.

Statistics update is done via the same executor (but statistics update after table creation or modification is more critical than statistics delete).

.../main/java/org/apache/ignite/internal/processors/datastructures/DataStructuresProcessor.java

modules/core/src/main/java/org/apache/ignite/internal/processors/task/GridTaskProcessor.java

...core/src/main/java/org/apache/ignite/internal/managers/encryption/GridEncryptionManager.java

...rg/apache/ignite/internal/processors/metastorage/persistence/DistributedMetaStorageImpl.java

...les/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/RootQuery.java

Vladsz83 · 2024-08-14T15:50:00Z

...c/main/java/org/apache/ignite/internal/processors/query/schema/management/SchemaManager.java

@@ -451,6 +454,9 @@ public void onCacheDestroyed(String cacheName, boolean rmvIdx, boolean clearIdx)
            if (schema.decrementUsageCount()) {
                schemas.remove(schemaName);

+                if (destroy)


We just removed local schema and called onSchemaDropped(). Why do we keep cluster schema if cache was only stopped? Looks wierd.

I didn't get this comment.
We drop views on cache destroy, on cache stop we shouldn't drop the views.

# Conflicts: # modules/core/src/main/java/org/apache/ignite/internal/processors/cluster/GridClusterStateProcessor.java # modules/core/src/main/java/org/apache/ignite/internal/processors/datastructures/DataStructuresProcessor.java

...va/org/apache/ignite/internal/processors/subscription/GridInternalSubscriptionProcessor.java

...e/src/main/java/org/apache/ignite/internal/processors/query/calcite/schema/IgniteSchema.java

Vladsz83 · 2024-08-28T11:17:11Z

...e/src/main/java/org/apache/ignite/internal/processors/query/calcite/schema/IgniteSchema.java

@@ -41,6 +41,9 @@ public class IgniteSchema extends AbstractSchema {
    /** */
    private final Multimap<String, Function> funcMap = Multimaps.synchronizedMultimap(HashMultimap.create());

+    /** */
+    private final Map<String, String> viewMap = new ConcurrentHashMap<>();


For the tables and the functions we store interfaces like Function or IgniteTable. For views a string/sql/request instead. Could we similary keep something like TableMacro. If I'm right, function register would not be required in that case.

TableMacro requires schamaPlus. We don't have a schemaPlus until register method, so we can't use table macro here.

Vladsz83 · 2024-08-28T14:28:05Z

...alcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/util/TypeUtils.java

+
+        while (true) {
+            if (F.isEmpty(origin) || cnt++ >= 100)
+                return typeFactory.getResultClass(type);


An exception may be like "Unsupported views depth"? Should we limit this threshold somewhere at a view creation? Like MAX_VIEW_DEPTH = 10?

We already have a check for recursive views, I'm not sure we should limit count of inner views.
I think this case will never happens (since we prohibit recursive views), but this condition is just for our own peace of mind.
I've indroduced new variable MAX_VIEW_DEPTH instead of hardcoded value.

What happens if the threshold is reached? A wrong type? A type coercion or a cast error? Do we have a test for it? I tried MAX_VIEW_DEPTH == 1, MAX_VIEW_DEPTH == 0. All the related tests seem to work.

...c/test/java/org/apache/ignite/internal/processors/query/calcite/sql/SqlCustomParserTest.java

...pache/ignite/internal/processors/query/calcite/integration/AuthorizationIntegrationTest.java

Vladsz83 · 2024-08-28T15:56:33Z

...va/org/apache/ignite/internal/processors/query/calcite/integration/ViewsIntegrationTest.java

+    }
+
+    /** {@inheritDoc} */
+    @Override protected void beforeTestsStarted() throws Exception {


Let's remove this method

We need to override this method, since we need to control grids lifecycle in each test

...va/org/apache/ignite/internal/processors/query/calcite/integration/ViewsIntegrationTest.java

Vladsz83 · 2024-08-28T16:25:09Z

...va/org/apache/ignite/internal/processors/query/calcite/integration/ViewsIntegrationTest.java

+        client.destroyCache("CACHE1");
+
+        assertViewsCount("PUBLIC", "MY_VIEW", 1);
+        assertViewsCount("CACHE1", "MY_VIEW", 0);


Why we remove a schema when it contains a view which requests an existing tabe/cache? There a VIEW which calls existing public.my_table

We don't have dedicated schema management procedures, except cache create/destroy. When schema is dropped (force by cache destroy) - it's assumed that schema objects is dropped too.

Vladsz83 · 2024-08-28T16:29:05Z

...va/org/apache/ignite/internal/processors/query/calcite/integration/ViewsIntegrationTest.java

+        initGrids(3);
+        initTable(3);
+
+        assertThrows("CREATE VIEW my_schema.my_view AS SELECT * FROM public.my_table", IgniteSQLException.class,


Let's check full message or at least a part from it.

There are different messages for calcite and H2 engines, so only common part is checked here

Vladsz83 · 2024-08-28T16:32:26Z

...va/org/apache/ignite/internal/processors/query/calcite/integration/ViewsIntegrationTest.java

+     * Tests views information distribution on node join.
+     */
+    @Test
+    public void testNodeJoin() throws Exception {


What if only non-baseline node left? Will a view exist?

Distributed metastorage is stored on non-baseline nodes too (AFAIK)

...org/apache/ignite/internal/processors/query/calcite/planner/UserDefinedViewsPlannerTest.java

...e/src/main/java/org/apache/ignite/internal/processors/query/schema/SchemaSqlViewManager.java

Vladsz83 · 2024-08-28T17:50:28Z

...e/src/main/java/org/apache/ignite/internal/processors/query/schema/SchemaSqlViewManager.java

+
+    /** */
+    public void clearSchemaViews(String schemaName) {
+        if (!U.isLocalNodeCoordinator(ctx.discovery()))


Let's use !U.isLocalNodeCoordinator(ctx.discovery()) outside

I disagree here.
SchemaSqlViewManager - it's a global manager, it's his responsibility to control global views state.
SchemaManager it's a local-node manager and shouldn't know about ways, how SchemaSqlViewManager store the views.

...e/src/main/java/org/apache/ignite/internal/processors/query/schema/SchemaSqlViewManager.java

modules/core/src/test/java/org/apache/ignite/internal/sql/SqlParserViewSelfTest.java

modules/core/src/main/java/org/apache/ignite/internal/sql/command/SqlDropViewCommand.java

.../main/java/org/apache/ignite/internal/processors/query/schema/management/ViewDescriptor.java

...c/main/java/org/apache/ignite/internal/processors/query/schema/management/SchemaManager.java

...alcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/util/TypeUtils.java

...org/apache/ignite/internal/processors/query/calcite/planner/UserDefinedViewsPlannerTest.java

# Conflicts: # modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/sql/generated/IgniteSqlParserImpl.java

sonarqubecloud · 2024-09-24T18:09:09Z

Quality Gate failed

Failed conditions
13 New Code Smells (required ≤ 1)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

alex-plekhanov force-pushed the ignite-22717 branch 2 times, most recently from 0eec8ce to 30fc18d Compare August 8, 2024 08:44

IGNITE-22717 SQL: User defined SQL views

b6d5f2b

alex-plekhanov force-pushed the ignite-22717 branch from 53f74e6 to b6d5f2b Compare August 8, 2024 16:32

Vladsz83 reviewed Aug 12, 2024

View reviewed changes

docs/_docs/sql-reference/ddl.adoc Outdated Show resolved Hide resolved

Vladsz83 reviewed Aug 12, 2024

View reviewed changes

...c/main/java/org/apache/ignite/internal/processors/query/calcite/schema/SchemaHolderImpl.java Show resolved Hide resolved

Vladsz83 reviewed Aug 12, 2024

View reviewed changes

...e/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/PlannerTest.java Outdated Show resolved Hide resolved

Vladsz83 reviewed Aug 13, 2024

View reviewed changes

...ain/java/org/apache/ignite/internal/processors/query/schema/management/SchemaDescriptor.java Outdated Show resolved Hide resolved

Vladsz83 reviewed Aug 13, 2024

View reviewed changes

...ain/java/org/apache/ignite/internal/processors/query/schema/management/SchemaDescriptor.java Outdated Show resolved Hide resolved

Vladsz83 reviewed Aug 13, 2024

View reviewed changes

Vladsz83 reviewed Aug 14, 2024

View reviewed changes

.../main/java/org/apache/ignite/internal/processors/datastructures/DataStructuresProcessor.java Outdated Show resolved Hide resolved

Vladsz83 reviewed Aug 14, 2024

View reviewed changes

modules/core/src/main/java/org/apache/ignite/internal/processors/task/GridTaskProcessor.java Outdated Show resolved Hide resolved

Vladsz83 reviewed Aug 14, 2024

View reviewed changes

...core/src/main/java/org/apache/ignite/internal/managers/encryption/GridEncryptionManager.java Outdated Show resolved Hide resolved

Vladsz83 reviewed Aug 14, 2024

View reviewed changes

...rg/apache/ignite/internal/processors/metastorage/persistence/DistributedMetaStorageImpl.java Outdated Show resolved Hide resolved

Vladsz83 reviewed Aug 14, 2024

View reviewed changes

...les/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/RootQuery.java Outdated Show resolved Hide resolved

Vladsz83 reviewed Aug 14, 2024

View reviewed changes