[Enhancement] skip optimizer lock for materialized view

[murphyatwork]

Fixes #issue

What type of PR is this:

• BugFix
• Feature
• Enhancement
• Refactor
• UT
• Doc
• Tool

Does this PR entail a change in behavior?

• Yes, this PR will result in a change in behavior.
• No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

• Interface/UI changes: syntax, type conversion, expression evaluation, display information
• Parameter changes: default values, similar parameters but with different default values
• Policy changes: use new policy to replace old one, functionality automatically enabled
• Feature removed
• Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

• I have added test cases for my bug fix or my new feature
• This pr needs user documentation (for new or modified features or behaviors)
  • I have added documentation for my new feature or new function

Bugfix cherry-pick branch check:

• I have checked the version labels which the pr will be auto-backported to the target branch
  • 3.2
  • 3.1
  • 3.0
  • 2.5

[starrocks-cr]

The code snippet you've provided appears to be from a Java application that deals with refreshing schemes and query copying for objects related to database tables or Materialized Views. The key points from the given code are:

1. Implementation of the copy() method in the MvRefreshScheme class.
2. Overrides of the copyOnlyForQuery() method in what looks like an extension or specialization of the OlapTable class, possibly named MaterializedView.

Here are some suggestions for the code review:

Cloning/Copying Objects:

• In the copy() method of MvRefreshScheme, you are performing a shallow copy for the fields type and lastRefreshTime, which is fine if they are immutable.
• However, if the asyncRefreshContext has mutable state that might be changed elsewhere, then its copy() method also needs to ensure it performs a deep copy to avoid shared mutable state issues.

Potential Null Dereference:

• When calling this.asyncRefreshContext.copy(), it seems you're assuming copy() exists and is public. Ensure that this method is indeed present and properly implements the deep copy if necessary. If asyncRefreshContext can be null, which seems to be checked, make sure that its copy method handles null values accordingly or isn't called on a null reference.

Type Safety:

• Using raw types like Sets.newHashSet(this.baseTableIds) and Lists.newArrayList(this.baseTableInfos) is type-safe, but it doesn't inform about the type of elements these collections contain. Use generic types if possible for better compile-time type checking, e.g., Sets.<Type>newHashSet(this.baseTableIds) where Type is the actual data type of elements in baseTableIds.

Cost of Copying:

• Methods like copyOnlyForQuery() perform the copying of various data structures like sets and lists. Consider the size of these structures and the frequency of the copy operation as it could be costly in terms of performance.

Design/Architecture:

• Deep copying usually indicates that there's a significant alteration between instances that requires a new instance to be isolated from the original one. Reevaluate whether there's room for improvement in sharing data instead of copying if possible (immutable objects, shared references with no mutations, etc.), which can reduce memory footprint and improve performance.

General Coding Practices:

• Always check any other object fields (like in MaterializedView) that might also require copying.
• Wherever applicable, consider implementing Cloneable interface and use clone() method (with proper handling of CloneNotSupportedException), although it should be noted that clone() has its own set of pitfalls and is not commonly recommended.
• Document the behavior clearly where a method does a deep copy vs. a shallow copy, outlining the implications of using each.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
9 Code Smells

0.0% Coverage
0.0% Duplication

The version of Java (11.0.21) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

[github-actions]

[BE Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

[github-actions]

[FE Incremental Coverage Report]

✅ pass : 53 / 57 (92.98%)

file detail

	path	covered_line	new_line	coverage	not_covered_line_detail
🔵	com/starrocks/catalog/OlapTable.java	4	6	66.67%	[322, 325]
🔵	com/starrocks/catalog/MaterializedView.java	18	19	94.74%	[908]
🔵	com/starrocks/sql/analyzer/AnalyzerUtils.java	19	20	95.00%	[616]
🔵	com/starrocks/scheduler/PartitionBasedMvRefreshProcessor.java	2	2	100.00%	[]
🔵	com/starrocks/sql/StatementPlanner.java	10	10	100.00%	[]

[github-actions]

@Mergifyio backport branch-3.2

[github-actions]

@Mergifyio backport branch-3.1

[github-actions]

@Mergifyio backport branch-3.0

[github-actions]

@Mergifyio backport branch-2.5

[mergify]

backport branch-3.2

✅ Backports have been created

• #34846 [Enhancement] skip optimizer lock for materialized view (backport #34569) has been created for branch branch-3.2

[mergify]

backport branch-3.1

✅ Backports have been created

• #34847 [Enhancement] skip optimizer lock for materialized view (backport #34569) has been created for branch branch-3.1 but encountered conflicts

[mergify]

backport branch-3.0

✅ Backports have been created

• #34848 [Enhancement] skip optimizer lock for materialized view (backport #34569) has been created for branch branch-3.0 but encountered conflicts

[mergify]

backport branch-2.5

✅ Backports have been created

• #34849 [Enhancement] skip optimizer lock for materialized view (backport #34569) has been created for branch branch-2.5 but encountered conflicts