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

test facility: build BoxedExpression from "pretty" representation of expression #7881

Closed
stdrc opened this issue Feb 13, 2023 · 3 comments · Fixed by #8774
Closed

test facility: build BoxedExpression from "pretty" representation of expression #7881

stdrc opened this issue Feb 13, 2023 · 3 comments · Fixed by #8774
Assignees

Comments

@stdrc
Copy link
Member

stdrc commented Feb 13, 2023

Is your feature request related to a problem? Please describe.

We now have plenty of test cases and benchmark cases building BoxedExpression with build_from_prost and hand-constructed ExprNode. The lack of function like build_expression_from_pretty("$0 > 10") makes writing unit tests painful.

An example:

    // in src/batch/benches/filter.rs

    // Expression: $1 % 2 == 0
    let expr = {
        let input_ref1 = ExprNode {
            expr_type: InputRef as i32,
            return_type: Some(risingwave_pb::data::DataType {
                type_name: TypeName::Int64 as i32,
                ..Default::default()
            }),
            rex_node: Some(RexNode::InputRef(InputRefExpr { column_idx: 0 })),
        };

        let literal2 = ExprNode {
            expr_type: TConstValue as i32,
            return_type: Some(risingwave_pb::data::DataType {
                type_name: TypeName::Int64 as i32,
                ..Default::default()
            }),
            rex_node: Some(RexNode::Constant(ProstDatum {
                body: serialize_datum(Some(ScalarImpl::Int64(2)).as_ref()),
            })),
        };

        // $1 % 2
        let mod2 = ExprNode {
            expr_type: Modulus as i32,
            return_type: Some(risingwave_pb::data::DataType {
                type_name: TypeName::Int64 as i32,
                ..Default::default()
            }),
            rex_node: Some(RexNode::FuncCall(FunctionCall {
                children: vec![input_ref1, literal2],
            })),
        };

        let literal0 = ExprNode {
            expr_type: TConstValue as i32,
            return_type: Some(risingwave_pb::data::DataType {
                type_name: TypeName::Int64 as i32,
                ..Default::default()
            }),
            rex_node: Some(RexNode::Constant(ProstDatum {
                body: serialize_datum(Some(ScalarImpl::Int64(0)).as_ref()),
            })),
        };

        // $1 % 2 == 0
        ExprNode {
            expr_type: Equal as i32,
            return_type: Some(risingwave_pb::data::DataType {
                type_name: TypeName::Boolean as i32,
                ..Default::default()
            }),
            rex_node: Some(RexNode::FuncCall(FunctionCall {
                children: vec![mod2, literal0],
            })),
        }
    };

    let expr = build_from_prost(&expr).unwrap();

If we have build_expression_from_pretty, this piece of code can be reduced to:

    let expr = build_expression_from_pretty("$1 % 2 == 0");

Describe the solution you'd like

I think a subset of SQL expression parser is enough, only allowing variables in the form of $n representing InputRef and basic operators.

Describe alternatives you've considered

No response

Additional context

No response

@github-actions github-actions bot added this to the release-0.1.17 milestone Feb 13, 2023
@xxchan
Copy link
Member

xxchan commented Feb 13, 2023

Or maybe we can introduce builder pattern or sth, which also applies to executors #3847 (comment)

@st1page
Copy link
Contributor

st1page commented Feb 13, 2023

Actually, I think it is not necessary to implement our current executor's "unit test" as the unit test in the streaming crate. The tests are just calling the public interface of the executors. So maybe we can implement the tests in another new crate so that we can import the frontend's code which includes the generating of frontend's expression with type inference. And then conver them into proto and BoxedExpression. We may even can call the parser's method in that crate!

@xxchan
Copy link
Member

xxchan commented Feb 13, 2023

I think that's ok. Current UTs are basically just ITs.

One caveat:🤪 #7842

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants