You are viewing a plain text version of this content. The canonical link for it is here.
Posted to codereview@trafodion.apache.org by selvaganesang <gi...@git.apache.org> on 2017/05/09 04:11:32 UTC
[GitHub] incubator-trafodion pull request #1087: [TRAFODION-2420] RMS enhancements
GitHub user selvaganesang opened a pull request:
https://github.com/apache/incubator-trafodion/pull/1087
[TRAFODION-2420] RMS enhancements
New counters introduced as part of BMO
InterimRowCount - Counter to keep track intermediate rows
produced by hash join and for intermediate
runs in sort
phase - BMO Phase for hash-join and sort
get statistics for statement s1, options 'cs' now works.
Removed attaching BMO stats to Non-root Paritial and leaf partition hash group by operators.
These operators are non-blocking BMOs.
Introduced 3 phases for hash grby operator to be visible in RMS Stats
options 'sl' now works with get statistics for qid.
The relevant new counters are now part of accumulated statistics type.
Extended the support upto 512 opeartors from 256 operators in RMS
Fixed both Type 2 and mxosrvr code to obtain all the externalized counters in accumualted stats correctly.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/selvaganesang/incubator-trafodion trafodion-2420_4
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-trafodion/pull/1087.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1087
----
commit e7719015286a0217fa5ec81551a783f8bc18fc3b
Author: selvaganesang <se...@esgyn.com>
Date: 2017-05-01T23:11:47Z
[TRAFODION-2420] RMS enhancements
New counters introduced as part of BMO
InterimRowCount - Counter to keep track intermediate rows
produced by hash join and for intermediate
runs in sort
phase - BMO Phase for hash-join and sort
get statistics for statement s1, options 'cs' now works.
Removed attaching BMO stats to Non-root Paritial and leaf partition hash group by operators.
These operators are non-blocking BMOs.
Introduced 3 phases for hash grby operator to be visible in RMS Stats
options 'sl' now works with get statistics for qid.
The relevant new counters are now part of accumulated statistics type.
Extended the support upto 512 opeartors from 256 operators in RMS
Fixed both Type 2 and mxosrvr code to obtain all the externalized counters in accumualted stats correctly.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request #1087: [TRAFODION-2420] RMS enhancements
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/1087#discussion_r115595056
--- Diff: core/sql/executor/ex_hash_grby.cpp ---
@@ -452,6 +463,8 @@ short ex_hash_grby_tcb::work() {
hasFreeTupp_ = TRUE;
};
};
+ if (bmoStats_)
+ bmoStats_->setBmoPhase(PHASE_END-HGB_READ_PHASE);
--- End diff --
This is executed for every row. Would it be cheaper to do this in the two places where we set state_ to HASH_GRBY_READ_CHILD?
Another question not about correctness but more related to personal taste: Why not simply assign values 1, 2, 3 etc. in the enum instead of subtracting two enums here? Feel free to ignore if you like this solution better.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request #1087: [TRAFODION-2420] RMS enhancements
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/1087#discussion_r115611428
--- Diff: core/sql/executor/ex_hash_grby.cpp ---
@@ -452,6 +463,8 @@ short ex_hash_grby_tcb::work() {
hasFreeTupp_ = TRUE;
};
};
+ if (bmoStats_)
+ bmoStats_->setBmoPhase(PHASE_END-HGB_READ_PHASE);
--- End diff --
With the lack of test case to test out the phase changes, I took a safer approach of setting it many times. I will move this code to the two places mentioned after I did some testing with TPCDS queries
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request #1087: [TRAFODION-2420] RMS enhancements
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/1087#discussion_r115612077
--- Diff: core/sql/executor/ex_hash_grby.h ---
@@ -302,6 +312,7 @@ NA_EIDPROC
void resetClusterAndReadFromChild(); // Tmobile.
public:
+ static const char *HashGrbyPhaseStr[];
--- End diff --
That's correct. Static global variables are also thread safe and I am hoping these phases are not changed forever and hence it shouldn't be error prone.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request #1087: [TRAFODION-2420] RMS enhancements
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/1087#discussion_r115616713
--- Diff: core/sql/executor/ex_hash_grby.cpp ---
@@ -452,6 +463,8 @@ short ex_hash_grby_tcb::work() {
hasFreeTupp_ = TRUE;
};
};
+ if (bmoStats_)
+ bmoStats_->setBmoPhase(PHASE_END-HGB_READ_PHASE);
--- End diff --
Thanks for the explanation!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request #1087: [TRAFODION-2420] RMS enhancements
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/1087#discussion_r115610081
--- Diff: core/sql/executor/ex_hash_grby.cpp ---
@@ -452,6 +463,8 @@ short ex_hash_grby_tcb::work() {
hasFreeTupp_ = TRUE;
};
};
+ if (bmoStats_)
+ bmoStats_->setBmoPhase(PHASE_END-HGB_READ_PHASE);
--- End diff --
For your latter question, I didn't want to disturb the existing enum values and hence kept the existing value. But, for BMO stats, I wanted show the phase of the instance that hasn't moved to the next higher phase. When the BMO stats is merged from different esp instances, higher phase value is used. Hence, the phase enum value is reversed for BMO stats
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request #1087: [TRAFODION-2420] RMS enhancements
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-trafodion/pull/1087
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request #1087: [TRAFODION-2420] RMS enhancements
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/1087#discussion_r115593353
--- Diff: core/sql/executor/ex_hash_grby.h ---
@@ -302,6 +312,7 @@ NA_EIDPROC
void resetClusterAndReadFromChild(); // Tmobile.
public:
+ static const char *HashGrbyPhaseStr[];
--- End diff --
Minor comment: One way to avoid the static, global variable would be to use a method that returns a constant rather than this array.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---