-
-
Notifications
You must be signed in to change notification settings - Fork 141
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
bug(tianmu):improve the code quality and refactoring derived table optimize for tianmu(#1277, #1276, #1258) #1281
Conversation
Thanks for the contribution! Please review the labels and make any necessary changes. |
This pull request's title should follow requirements next. @isredstar please check it 👇. Valid format:
Valid types:
|
3e93dce
to
7341a45
Compare
Codecov ReportBase: 0.00% // Head: 43.10% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## stonedb-5.7-dev #1281 +/- ##
====================================================
+ Coverage 0 43.10% +43.10%
====================================================
Files 0 1830 +1830
Lines 0 396311 +396311
====================================================
+ Hits 0 170839 +170839
- Misses 0 225472 +225472
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
7341a45
to
0531e62
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest to refer to this script to format your code.( you should have installed clang-format, clang-format-diff.py)
#/bin/bash
echo $0
cd ..
cur=`pwd`"/"
cd storage/tianmu
echo $cur
git status .|grep modified |awk '{print $2}'|xargs -i -n 1 clang-format --style=file -i {}
git diff HEAD -U0 --no-color --src-prefix=$cur --dst-prefix=$cur | clang-format-diff.py -i -style=file
including those in semi-joins. | ||
*/ | ||
if (select_lex->materialized_derived_table_count) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blank space number was more than expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
@@ -55,6 +55,15 @@ typedef struct st_rollup | |||
List<Item> *fields; | |||
} ROLLUP; | |||
|
|||
enum class OptimizePhase // for Tianmu to indicate which optimization phase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refer to googel style: https://google.github.io/styleguide/cppguide.html#Enumerator_Names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
sql/sql_optimizer.cc
Outdated
@@ -10444,14 +10435,14 @@ static bool internal_remove_eq_conds(THD *thd, Item *cond, | |||
} | |||
if (cond->const_item()) | |||
{ | |||
if (part!=1 || cond->type()!=Item::SUBSELECT_ITEM) | |||
{//TIANMU UPGRADE | |||
if (phase !=OptimizePhase::Before_LOJ_Transform || cond->type()!=Item::SUBSELECT_ITEM) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a blank space after '!="
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
0531e62
to
c4a4933
Compare
c4a4933
to
20358f9
Compare
sql/sql_lex.h
Outdated
int optimize_for_tianmu(); | ||
int optimize_after_tianmu(); | ||
//END | ||
int after_optimize_tianmu(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why rename the function name? what's the difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The optimize_after_tianmu
has been as a variable name,so I think it is not suitable as a function name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a function name should start with a verb. like set_value, update_row, I suggest rename the variable name.
a8637fe
to
3bf8723
Compare
c04a28b
to
64fc98c
Compare
64fc98c
to
3ef25d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary about this PR
Issue Number: close #1247,#1276,#1258
refactor derived table optimize for tianmu
improve the code quality to modify the magic number
Tests Check List
Changelog
Documentation