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

fix(expr): several expressions handle visibility incorrectly #3096

Merged
merged 6 commits into from
Jun 10, 2022

Conversation

TennyZhuang
Copy link
Collaborator

@TennyZhuang TennyZhuang commented Jun 9, 2022

Signed-off-by: TennyZhuang zty0826@gmail.com

What's changed and what's your intention?

PLEASE DO NOT LEAVE THIS EMPTY !!!

The visibility interface is error-prune, and I'll fix it in the later PR. I'm designing now.

For this PR, I introduced a helper function with_invisible_holes to simplify the construction of a DataChunk, then find and fix several bugs.

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

Refer to a related PR or issue link (optional)

Signed-off-by: TennyZhuang <zty0826@gmail.com>
@TennyZhuang TennyZhuang self-assigned this Jun 9, 2022
@github-actions github-actions bot added the type/fix Bug fix label Jun 9, 2022
Signed-off-by: TennyZhuang <zty0826@gmail.com>
Signed-off-by: TennyZhuang <zty0826@gmail.com>
Signed-off-by: TennyZhuang <zty0826@gmail.com>
Comment on lines +68 to +73
pub fn len(&self) -> usize {
match self {
Vis::Bitmap(b) => b.len(),
Vis::Compact(c) => *c,
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Shall we call it capacity?
  • Do we expect this Vis to expose the same interface as Bitmap? Essentially they are the same thing. Or maybe we should just refactor Bitmap to use either a compact usize or a Bytes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. I'd prefer len, capacity will bring some ambiguity.
  2. Generally I think yes, but it's a huge refactor.

Comment on lines +61 to +62
// TODO: we can avoid the compact here.
let input = input.clone().compact().map_err(ExprError::Array)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to avoid this compact, by replacing the 2 capacity with cardinality below. But it still panics with zip_eq unequal length in the expr template.

The root cause of these errors is this:
When arr = expr.eval(chunk) where the chunk has capacity = 5 and cardinality = 3, do we expect the returned arr to have length 5 or 3?

It is even inconsistent inside the expr template itself: it output arr with length 3 but expects its inner expr to return arr with length 5. This can be reproduced by the following unit test:

        use risingwave_pb::data::DataType as ProstDataType;
        use risingwave_pb::expr::expr_node::RexNode;
        use risingwave_pb::expr::{FunctionCall};

        let data_chunk = DataChunk::from_pretty(
            "i
             1
             3
             5 D
             7  ",
        );

        let input_ref = make_input_ref(0, TypeName::Int32);
        let return_type = ProstDataType {
            type_name: TypeName::Int32 as i32,
            ..Default::default()
        };
        let inner = ExprNode {
            expr_type: Type::Add as i32,
            return_type: Some(return_type.clone()),
            rex_node: Some(RexNode::FuncCall(FunctionCall {
                children: vec![input_ref.clone(), input_ref.clone()]
            })),
        };

        let expr = ExprNode {
            expr_type: Type::Add as i32,
            return_type: Some(return_type),
            rex_node: Some(RexNode::FuncCall(FunctionCall {
                children: vec![inner, make_input_ref(0, TypeName::Int32)]
            })),
        };

        // expr is `($0 + $0) + $0`

        let vec_executor = build_from_prost(&expr).unwrap();
        let res = vec_executor.eval(&data_chunk).unwrap();

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for no implicit compaction, i.e. 5->3 by expr.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch!
No implicit compaction is wasteful, but I guess that we can find a good way to do this!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd like to merge the PR first, at least it introduces some test helpers and fixes a part of the problems. I will try to fix it completely in the later PR.

Signed-off-by: TennyZhuang <zty0826@gmail.com>
@TennyZhuang TennyZhuang enabled auto-merge (squash) June 10, 2022 01:48
@TennyZhuang TennyZhuang merged commit df1c61d into main Jun 10, 2022
@TennyZhuang TennyZhuang deleted the fix/expr-visibility branch June 10, 2022 04:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants