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

Support transform exp tree into RPN #4329

Merged
merged 16 commits into from Mar 21, 2019

Conversation

@breeswish
Copy link
Member

commented Mar 7, 2019

What have you changed? (mandatory)

This PR adds RpnExpressionNodeVec, which can transform an expression tree into RPN form (a vector of RpnExpressionNode).

This PR is extracted from #3898.

What are the type of the changes? (mandatory)

  • New feature (change which adds functionality)

How has this PR been tested? (mandatory)

Unit test

@breeswish breeswish added the C: Copr label Mar 7, 2019

@breeswish breeswish referenced this pull request Mar 7, 2019

Merged

Introduce Vectorized Evaluation framework #4322

3 of 3 tasks complete
@zhouqiang-cl

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

/test

Show resolved Hide resolved src/coprocessor/dag/rpn_expr/types.rs Outdated
Show resolved Hide resolved src/coprocessor/dag/rpn_expr/types.rs Outdated
@AndreMouche

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

Please merge master and fix the conflicts.

@AndreMouche
Copy link
Member

left a comment

Should we consider to short a function if it is too long.

Show resolved Hide resolved src/coprocessor/dag/rpn_expr/types.rs Outdated
Show resolved Hide resolved src/coprocessor/dag/rpn_expr/types.rs Outdated
Show resolved Hide resolved src/coprocessor/dag/rpn_expr/types.rs Outdated
Show resolved Hide resolved src/coprocessor/dag/rpn_expr/types.rs Outdated
Show resolved Hide resolved src/coprocessor/dag/rpn_expr/types.rs Outdated

let node_fn_a_1 = {
// node b
let mut node_b = Expr::new();

This comment has been minimized.

Copy link
@crazycs520

crazycs520 Mar 14, 2019

Contributor

Can we extract to a function like this?

        fn new_expr(exprType: ExprType, fieldTypeTp: FieldTypeTp, childExprs: Vec<Expr>) -> Expr {
            let mut node = Expr::new();
            node.set_tp(exprType);
            node
                .mut_field_type()
                .as_mut_accessor()
                .set_tp(fieldTypeTp);
            for child in childExprs {
                node.mut_children().push(child);
            }
            node
        }

This comment has been minimized.

Copy link
@breeswish

breeswish Mar 14, 2019

Author Member

Yup. Planned to do this (named like KvprotoHelper or TipbHelper) after we merged all executors, providing some commonly used utilities.

Re-extract this PR
Signed-off-by: Breezewish <breezewish@pingcap.com>

@breeswish breeswish force-pushed the breeswish:___batch_extract/t3/2.5 branch from 895d7e8 to 5bd29a0 Mar 14, 2019

@breeswish

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2019

/run-integration-tests

@crazycs520
Copy link
Contributor

left a comment

LGTM

Show resolved Hide resolved src/coprocessor/dag/rpn_expr/types.rs Outdated
Show resolved Hide resolved src/coprocessor/dag/rpn_expr/types.rs Outdated
Show resolved Hide resolved src/coprocessor/dag/rpn_expr/types.rs Outdated

breeswish added some commits Mar 15, 2019

Rename some fields to be more clear
Signed-off-by: Breezewish <breezewish@pingcap.com>
Fix typo
Signed-off-by: Breezewish <breezewish@pingcap.com>
Make name more precise
Signed-off-by: Breezewish <breezewish@pingcap.com>

breeswish added some commits Mar 18, 2019

Merge master change
Signed-off-by: Breezewish <breezewish@pingcap.com>

@breeswish breeswish force-pushed the breeswish:___batch_extract/t3/2.5 branch from a599ac8 to ef84034 Mar 18, 2019

Let's make everyone happy
Signed-off-by: Breezewish <breezewish@pingcap.com>
// See the License for the specific language governing permissions and
// limitations under the License.

#![allow(dead_code)]

This comment has been minimized.

Copy link
@rleungx

rleungx Mar 18, 2019

Member

Can we remove it? Or better to add a TODO here?

This comment has been minimized.

Copy link
@breeswish

breeswish Mar 18, 2019

Author Member

Thanks for reminding. Currently the expression built is not used, so it is a dead code. We will remove it when the evaluation part get merged.

Further simplify the codebase
Signed-off-by: Breezewish <breezewish@pingcap.com>

breeswish added some commits Mar 19, 2019

@brson

brson approved these changes Mar 20, 2019

Copy link
Contributor

left a comment

LGTM

rleungx and others added some commits Mar 21, 2019

@breeswish

This comment has been minimized.

Copy link
Member Author

commented Mar 21, 2019

@brson Thanks a lot for the approval XD

@breeswish breeswish merged commit 5a0c02d into tikv:master Mar 21, 2019

2 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details

@breeswish breeswish deleted the breeswish:___batch_extract/t3/2.5 branch Mar 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.