coprocessor/dag:fix bug caused by use offset in expr #2064
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andelf
reviewed
Jul 21, 2017
| @@ -35,27 +35,23 @@ pub struct AggregationExecutor<'a> { | |||
| cursor: usize, | |||
| executed: bool, | |||
| ctx: Rc<EvalContext>, | |||
| cols: Vec<ColumnInfo>, | |||
| cols: Rc<Vec<ColumnInfo>>, | |||
| related_cols: Vec<usize>, // offset of related columns | |||
There was a problem hiding this comment.
related_cols_offset or sth.?
There was a problem hiding this comment.
Better to use name consistently.
| name | represent |
|---|---|
| column/column_info/info | Column Info |
| id/column_id | Column ID |
| offset | Column Offset |
choleraehyq
changed the title
hhkbp2
reviewed
Jul 24, 2017
src/coprocessor/executor/mod.rs
Outdated
| } | ||
|
|
||
| pub fn visit(&mut self, expr: &Expr) -> Result<()> { | ||
| if expr.get_tp() == ExprType::ColumnRef { | ||
| self.col_ids.insert(box_try!(expr.get_val().decode_i64())); | ||
| let offset = box_try!(expr.get_val().decode_i64()) as usize; | ||
| if offset < self.cols_len { |
There was a problem hiding this comment.
Return an error if offset invalid?
hhkbp2
reviewed
Jul 24, 2017
src/coprocessor/executor/mod.rs
Outdated
| @@ -53,6 +68,10 @@ impl ExprColumnRefVisitor { | |||
| } | |||
| Ok(()) | |||
| } | |||
|
|
|||
| pub fn cols(self) -> Vec<usize> { | |||
hhkbp2
reviewed
Jul 24, 2017
src/coprocessor/executor/topn.rs
Outdated
| use super::super::table_scan::TableScanExecutor; | ||
| use super::super::scanner::test::{TestStore, get_range, new_col_info}; | ||
|
|
||
|
|
|
CI fails @AndreMouche |
|
Rest LGTM |
|
PTAL @hanfei1991 @XuHuaiyu @andelf |
|
LGTM |
BusyJay
reviewed
Jul 26, 2017
| ExprColumnRefVisitor { col_ids: HashSet::new() } | ||
| pub fn new(cols_len: usize) -> ExprColumnRefVisitor { | ||
| ExprColumnRefVisitor { | ||
| cols_offset: HashSet::default(), |
There was a problem hiding this comment.
No associated item named with_capacity found for type std::collections::HashSet<_, std::hash::BuildHasherDefault<fnv::FnvHasher>> in the current scope @BusyJay
src/coprocessor/executor/mod.rs
Outdated
| } | ||
|
|
||
| pub fn visit(&mut self, expr: &Expr) -> Result<()> { | ||
| if expr.get_tp() == ExprType::ColumnRef { | ||
| self.col_ids.insert(box_try!(expr.get_val().decode_i64())); | ||
| let offset = box_try!(expr.get_val().decode_i64()) as usize; | ||
| if offset > self.cols_len { |
src/coprocessor/executor/mod.rs
Outdated
| @@ -27,18 +36,28 @@ pub mod aggregation; | |||
|
|
|||
| #[allow(dead_code)] | |||
src/coprocessor/executor/mod.rs
Outdated
| self.col_ids.insert(box_try!(expr.get_val().decode_i64())); | ||
| let offset = box_try!(expr.get_val().decode_i64()) as usize; | ||
| if offset > self.cols_len { | ||
| return Err(Error::Other(box_err!("offset {} overflow,should be less than {}", |
| src: Box<Executor + 'a>) | ||
| -> Result<AggregationExecutor<'a>> { | ||
| // collect all cols used in aggregation | ||
| let mut visitor = ExprColumnRefVisitor::new(); | ||
| let mut visitor = ExprColumnRefVisitor::new(columns.len()); | ||
| let group_by = meta.take_group_by().into_vec(); |
There was a problem hiding this comment.
I think visitor.batch_visit(meta.get_group_by()) should be ok.
There was a problem hiding this comment.
group_by is not only been used in visitor and would be taken later @BusyJay
| src: Box<Executor + 'a>) | ||
| -> Result<SelectionExecutor<'a>> { | ||
| let conditions = meta.take_conditions().into_vec(); | ||
| let mut visitor = ExprColumnRefVisitor::new(); | ||
| let mut visitor = ExprColumnRefVisitor::new(columns_info.len()); | ||
| for cond in &conditions { |
|
@BusyJay @hhkbp2 @hanfei1991 PTAL |
BusyJay
reviewed
Jul 27, 2017
tests/coprocessor/test_select.rs
Outdated
| @@ -193,6 +197,13 @@ impl Table { | |||
| c_info.set_pk_handle(col.id == self.handle_id); | |||
| idx_info.mut_columns().push(c_info); | |||
| } | |||
| if let Some(col) = self.cols.get(&self.handle_id) { | |||
BusyJay
reviewed
Jul 27, 2017
| let columns_info = index_info.take_columns(); | ||
| scan.set_columns(columns_info.clone()); | ||
| exec.set_idx_scan(scan); | ||
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi,
This PR will fix bug caused by use column_id instead of column_offset in expression.
And its tests in test_select is not finished.
@hanfei1991 you can test it first.