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

Update statement route #12316

Merged
merged 6 commits into from
Sep 10, 2021
Merged

Update statement route #12316

merged 6 commits into from
Sep 10, 2021

Conversation

soulasuna
Copy link
Contributor

Add update statement route.

For #11661.

Changes proposed in this pull request:

  • Add update case example.
  • Refactor shadow determiner.
  • Refactor abstract shadow route engine.
  • Add shadow update statement routing engine.
  • Fix shadow rule configuration yaml swapper.
  • Fix shadow ut.

* @param expression binary operation expression
* @return column name
*/
public static Optional<String> extractColumnName(final BinaryOperationExpression expression) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@strongduanmu Please look at this coding, do you think we can get all necessary info from statementContext

Copy link
Member

Choose a reason for hiding this comment

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

@strongduanmu Please look at this coding, do you think we can get all necessary info from statementContext

@tristaZero We already have a tool to extract columns based on expression, and we can use ColumnExtractor to replace this logic.

@tristaZero tristaZero added this to the 5.0.0-RC1 milestone Sep 9, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #12316 (30f7807) into master (e5b1ba3) will decrease coverage by 0.08%.
The diff coverage is 29.41%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #12316      +/-   ##
============================================
- Coverage     63.73%   63.64%   -0.09%     
- Complexity     1260     1273      +13     
============================================
  Files          2351     2364      +13     
  Lines         35754    35963     +209     
  Branches       6211     6253      +42     
============================================
+ Hits          22787    22889     +102     
- Misses        11142    11220      +78     
- Partials       1825     1854      +29     
Impacted Files Coverage Δ
...e/shardingsphere/shadow/route/ShadowSQLRouter.java 88.88% <0.00%> (+7.25%) ⬆️
...ure/engine/determiner/ShadowDeterminerFactory.java 50.00% <ø> (-12.50%) ⬇️
...miner/algorithm/NoteShadowAlgorithmDeterminer.java 0.00% <0.00%> (-100.00%) ⬇️
...engine/dml/ShadowDeleteStatementRoutingEngine.java 0.00% <ø> (ø)
...engine/dml/ShadowSelectStatementRoutingEngine.java 0.00% <ø> (ø)
...engine/dml/ShadowUpdateStatementRoutingEngine.java 0.00% <0.00%> (ø)
...ngine/impl/ShadowNonMDLStatementRoutingEngine.java 0.00% <ø> (ø)
...adow/route/future/engine/util/ShadowExtractor.java 0.00% <0.00%> (ø)
...ner/algorithm/ColumnShadowAlgorithmDeterminer.java 58.82% <25.00%> (-31.18%) ⬇️
...route/future/engine/AbstractShadowRouteEngine.java 35.71% <41.17%> (-55.96%) ⬇️
... and 98 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5b1ba3...30f7807. Read the comment docs.

* @param expression binary operation expression
* @return column name
*/
public static Optional<String> extractColumnName(final BinaryOperationExpression expression) {
Copy link
Member

Choose a reason for hiding this comment

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

@strongduanmu Please look at this coding, do you think we can get all necessary info from statementContext

@tristaZero We already have a tool to extract columns based on expression, and we can use ColumnExtractor to replace this logic.

}

@Override
protected ShadowDetermineCondition createShadowDetermineCondition() {
Copy link
Member

Choose a reason for hiding this comment

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

@soulasuna Why not consider extracting a ShadowConditionEngine to handle these logics like ShardingConditionEngine?

@strongduanmu strongduanmu merged commit 0a710ab into apache:master Sep 10, 2021
@soulasuna soulasuna deleted the update-statement-route branch September 10, 2021 09:27
@menghaoranss menghaoranss modified the milestones: 5.0.0-RC1, 5.0.0 Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants