Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Base refresh materialized view #800

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Gerych1984
Copy link

Q A
Is bugfix?
New feature? ✔️
Breaks BC?

@Gerych1984 Gerych1984 force-pushed the prepare-pgsql-mat-view branch from d38325b to 020d468 Compare January 22, 2024 11:52
Copy link

codecov bot commented Jan 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.64%. Comparing base (ef81e8b) to head (4e86a29).
Report is 9 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##             master     #800   +/-   ##
=========================================
  Coverage     99.64%   99.64%           
- Complexity     1271     1277    +6     
=========================================
  Files            63       63           
  Lines          3106     3119   +13     
=========================================
+ Hits           3095     3108   +13     
  Misses           11       11           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Gerych1984 Gerych1984 force-pushed the prepare-pgsql-mat-view branch from 020d468 to 348f586 Compare January 22, 2024 12:05
@Gerych1984 Gerych1984 force-pushed the prepare-pgsql-mat-view branch from 348f586 to 4e86a29 Compare January 22, 2024 12:19
@Gerych1984 Gerych1984 marked this pull request as ready for review January 22, 2024 12:40
@vjik vjik requested review from Tigrov and a team January 22, 2024 13:02
@vjik vjik added the status:code review The pull request needs review. label Jan 22, 2024
Copy link
Member

@Tigrov Tigrov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think materialized views should be added in complex. With create, alter, drop and refresh methods. And take into account features of all supported DBMSs.

So, we should realize all these methods for all supported DBMSs which support materialized views.

@Gerych1984
Copy link
Author

I think materialized views should be added in complex. With create, alter, drop and refresh methods. And take into account features of all supported DBMSs.

So, we should realize all these methods for all supported DBMSs which support materialized views.

That's what this PR is made for. Without it, i can't go any further

* @param bool|null $withData
* @return bool
*/
public function refreshMaterializedView(string $viewName, ?bool $concurrently = null, ?bool $withData = null): bool;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function refreshMaterializedView(string $viewName, ?bool $concurrently = null, ?bool $withData = null): bool;
public function refreshMaterializedView(string $viewName, bool $concurrently = false, ?bool $withData = null): bool;

Why the interface is different from DDLQueryBuilderInterface::refreshMaterializedView()?
Looks like should be the same.

When execute $command->refreshMaterializedView($viewName); expect this is not concurently.

Copy link

@smirnov-e smirnov-e May 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I see in MS SQL / Oracle / Clickhouse docs:

  1. WITH [NO] DATA is PostgreSQL only option and pretty much useless one.
  2. REFRESH exists only on PG and Oracle. MS SQL and Clickhouse do automatic updates, so their implementations should always return true.
    There are a lot of variants in Oracle: 3 commands (REFRESH, REFREASH_ALL_MVIEWS, REFRESH_DEPENDENT) and at least 6 options. Option out_of_place seems to be equivalent of CONCURRENTLY in PostgreSQL.

So, the most common part is refresh + option for non-blocking update, but it will be very hard to take full advantage of database capabilities like Oracle's refresh method.

Comment on lines +367 to +368
$sql = $db->getQueryBuilder()->refreshMaterializedView($viewName, $concurrently, $withData);
$actual = DbHelper::replaceQuotes($sql, $driver);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$sql = $db->getQueryBuilder()->refreshMaterializedView($viewName, $concurrently, $withData);
$actual = DbHelper::replaceQuotes($sql, $driver);
$actual = $db->getQueryBuilder()->refreshMaterializedView($viewName, $concurrently, $withData);

Result from $db->getQueryBuilder()->refreshMaterializedView() already quoted acording to the driver

@@ -178,7 +178,7 @@ public function testGetExpressionBuilderException(): void

$this->expectException(Exception::class);

$expression = new class () implements ExpressionInterface {
$expression = new class() implements ExpressionInterface {
Copy link
Member

@Tigrov Tigrov Jan 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Tigrov Tigrov added this to the 2.0.0 milestone Jan 31, 2024
@Tigrov
Copy link
Member

Tigrov commented Jan 31, 2024

Added this to the 2.0.0 milestone due to (almost) any change of interfaces breaks BC

*
* @return string The `REFRESH MATERIALIZED VIEW` SQL statement
*/
public function refreshMaterializedView(string $viewName, bool $concurrently = false, ?bool $withData = null): string;
Copy link
Member

@Tigrov Tigrov Jan 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function refreshMaterializedView(string $viewName, bool $concurrently = false, ?bool $withData = null): string;
public function refreshMaterializedView(string $viewName, bool $concurrently = false): string;

Not sure the argument $withData is needed. If it is false, it is equal to "truncate" the materialized view. Maybe better remove it and if the method is required add truncateMaterializedView()

Comment on lines +731 to +755
public static function refreshMaterializedViewDataProvider(): array
{
return [
[
'default_mt',
null,
null,
],
[
'concurrently_mt',
true,
null,
],
[
'concurrently_with_data_mt',
true,
true,
],
[
'concurrently_without_data_mt',
true,
false,
],
];
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NotSupportedException can be tested one time with one set of arguments.

Comment on lines +315 to +355
public static function refreshMaterializedViewDataProvider(): array
{
return [
[
'concurrently_mt',
true,
null,
'REFRESH MATERIALIZED VIEW CONCURRENTLY [[concurrently_mt]]',
],
[
'concurrently_with_data_mt',
true,
true,
'REFRESH MATERIALIZED VIEW CONCURRENTLY [[concurrently_with_data_mt]] WITH DATA',
],
[
'concurrently_without_data_mt',
true,
false,
'REFRESH MATERIALIZED VIEW CONCURRENTLY [[concurrently_without_data_mt]] WITH NO DATA',
],
[
'not_concurrently_mt',
false,
null,
'REFRESH MATERIALIZED VIEW [[not_concurrently_mt]]',
],
[
'not_concurrently_with_data_mt',
false,
true,
'REFRESH MATERIALIZED VIEW [[not_concurrently_with_data_mt]] WITH DATA',
],
[
'not_concurrently_without_data_mt',
false,
false,
'REFRESH MATERIALIZED VIEW [[not_concurrently_without_data_mt]] WITH NO DATA',
],
];
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to move it to QueryBuilderProvider

* @param bool|null $withData
* @return void
*/
public function testRefreshMaterializedView(string $viewName, bool $concurrently, ?bool $withData, string $expected): void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks need to move this test to AbstractQueryBuilderTest

@Tigrov Tigrov added status:under development Someone is working on a pull request. and removed status:code review The pull request needs review. labels Apr 6, 2024
@smirnov-e
Copy link

smirnov-e commented May 5, 2024

With create, alter, drop and refresh methods.

From what I see in MS SQL / Oracle / Clickhouse / PostgreSQL docs:

DROP: PostgreSQL requires DROP MATERIALIZED VIEW, other databases use simple DROP VIEW.

ALTER - quite different one from another

  • MS SQL only allows enable/disable MV
  • PostgreSQL allows modify name / column names / other options excluding source query
  • Clickhouse only allows to change source query
  • Oracle — refresh mode and type, enable/disable.

CREATE — MS SQL, PostgreSQL and Oracle has common syntax part, but Oracle has lots of useful extensions. All except MS SQL have option to skip data population (WITH NO DATA).

  • PostgreSQL — column names, storage options and other.
  • MS SQL — data distribution
  • Oracle — additional query for faster refresh, syntax for automatic refresh and partitioning.
  • Clickhouse — just query and SQL SECURITY.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:under development Someone is working on a pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants