IGNITE-20020: Sql. Support system views in planner. #2601

lowka · 2023-09-18T15:06:44Z

Makes system views accessible to planner via calcite's Table class.

Introduces IgniteDataSource interface that represents both Tables and System Views.
Extracts common fields from IgniteTable to IgniteDataSource.

https://issues.apache.org/jira/browse/IGNITE-20020

Thank you for submitting the pull request.

To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:

The Review Checklist

Formal criteria: TC status, codestyle, mandatory documentation. Also make sure to complete the following:
- 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 description of the JIRA ticket explains WHAT was made, WHY and 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.
Design: new code conforms with the design principles of the components it is added to.
Patch quality: patch cannot be split into smaller pieces, its size must be reasonable.
Code quality: code is clean and readable, necessary developer documentation is added if needed.
Tests code quality: test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.

Notes

Apache Ignite Coding Guidelines

ygerzhedovich · 2023-09-20T11:15:24Z

...sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rel/IgniteSystemViewScan.java

+ */
+public class IgniteSystemViewScan extends ProjectableFilterableTableScan implements SourceAwareIgniteRel {
+
+    private final long sourceId;


could you please add a javadoc with an explanation what is it

Filed an issue to document SourceIgniteRel - https://issues.apache.org/jira/browse/IGNITE-20461

ygerzhedovich · 2023-09-20T11:16:04Z

...sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rel/IgniteSystemViewScan.java

+        if (srcIdObj != null) {
+            sourceId = ((Number) srcIdObj).longValue();
+        } else {
+            sourceId = -1;


so many times used -1, let's introduce constant for it

May be we can create an issue to fix this for all scan operations?

yes, it good idea

Here is an issue https://issues.apache.org/jira/browse/IGNITE-20461

ygerzhedovich · 2023-09-20T14:04:28Z

...gine/src/main/java/org/apache/ignite/internal/sql/engine/schema/CatalogSqlSchemaManager.java

+            String viewName = systemViewDescriptor.name();
+            TableDescriptor descriptor = createTableDescriptorForSystemView(systemViewDescriptor);
+
+            IgniteSystemView schemaTable = new IgniteSystemViewImpl(viewName, viewId, version, descriptor, Statistics.UNKNOWN);


Is seems we could have special statistics realization for system views. At least we know distribution

Unfortunately, there is no statistics for rows for system views, because the API asks a user to provide it a function that returns data (see SystemView's dataProvider property).

yes, but statistics it not only about concrete rows, it also knowns about data structure and distribution, both of them don't depends on dataprovider

Thanks. Fixed.

Fix statistics.

Fix check style issues.

zstan · 2023-09-20T06:31:03Z

...gine/src/main/java/org/apache/ignite/internal/sql/engine/metadata/IgniteFragmentMapping.java


+    @Override
+    public FragmentMapping visit(IgniteSystemViewScan rel) {
+        throw new UnsupportedOperationException();


Suggested change

throw new UnsupportedOperationException();

throw new UnsupportedOperationException("System view scan is not implemented");

zstan · 2023-09-20T07:27:27Z

...ngine/src/test/java/org/apache/ignite/internal/sql/engine/planner/SystemViewPlannerTest.java

+    private static final AtomicInteger SYSTEM_VIEW_ID = new AtomicInteger();
+
+    @Test
+    public void testReadAllColumns() throws Exception {


assertPlan("INSERT into SYS_PROPS(key, VAL) VALUES('', '')"
throws confused exception, seems all cases with VIEWS mutations need to be covered.

CREATE INDEX idx on SYS_PROPS
the same i suppose

There is no facilities to add such tests - there is no means to write DDL tests for the planner.

assertPlan("INSERT into SYS_PROPS(key, VAL) VALUES('', '')"

Fixed.

zstan · 2023-09-20T07:30:15Z

...l-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteSqlValidator.java

+        assert dataSource != null;
+
+        if (dataSource instanceof IgniteSystemView) {
+            throw newValidationError(identifier, IgniteResource.INSTANCE.systemViewIsNotModifiable(identifier.toString()));


I think that SysView mutation error need to be raised not from getIgniteTable call, wdyt ?

Renamed to getTableForModification.

Fix INSERT, Rename get*Table methods. Add error message.

lowka force-pushed the IGNITE-20020 branch 8 times, most recently from 9267a27 to 6dcc608 Compare September 19, 2023 19:28

IGNITE-20020: Sql. Support system views in planner.

a6c13c5

lowka force-pushed the IGNITE-20020 branch from 6dcc608 to a6c13c5 Compare September 20, 2023 04:04

lowka marked this pull request as ready for review September 20, 2023 05:38

korlov42 approved these changes Sep 20, 2023

View reviewed changes

ygerzhedovich reviewed Sep 20, 2023

View reviewed changes

lowka added 3 commits September 20, 2023 17:26

IGNITE-20017: Introduce new command to catalog to create system view.

bae23f0

Fix statistics.

IGNITE-20017: Introduce new command to catalog to create system view.

7b84840

Fix check style issues.

Merge branch 'main' into IGNITE-20020

e0a46ef

lowka requested a review from ygerzhedovich September 21, 2023 12:28

zstan reviewed Sep 26, 2023

View reviewed changes

IGNITE-20020: Sql. Support system views in planner.

0bcd5e5

Fix INSERT, Rename get*Table methods. Add error message.

lowka requested a review from zstan September 26, 2023 12:10

zstan approved these changes Sep 26, 2023

View reviewed changes

zstan merged commit 11703c3 into apache:main Sep 27, 2023

zstan deleted the IGNITE-20020 branch September 27, 2023 11:11

	throw new UnsupportedOperationException();
	throw new UnsupportedOperationException("System view scan is not implemented");

IGNITE-20020: Sql. Support system views in planner. #2601

IGNITE-20020: Sql. Support system views in planner. #2601

Uh oh!

Conversation

lowka commented Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The Review Checklist

Notes

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!

Choose a reason for hiding this comment

Uh oh!

lowka Sep 20, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

lowka Sep 26, 2023 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lowka commented Sep 18, 2023 •

edited

Loading

lowka Sep 20, 2023 •

edited

Loading

lowka Sep 26, 2023 •

edited

Loading