-
Notifications
You must be signed in to change notification settings - Fork 590
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
feat(optimizer): refactor table desc && SeqScan && SeqScan pk derive && SeqScan serialize (#1250) #1258
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest LGTM.
let fields = required_col_idx | ||
.iter() | ||
.enumerate() | ||
.map(|(op_idx, tb_idx)| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is op_idx
the index of the output? Please use a more detailed name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's ask the original PR author 🤪
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @st1page
…&& SeqScan serialize
d8b7e3e
to
3a21ce1
Compare
Signed-off-by: Alex Chi <iskyzh@gmail.com>
I will fix them in the another PR, and we will merge this first to not blocking others |
What's changed and what's your intention?
the
LogicalScan
BatchScan
StreamScan
has not enough information to serialize to a plan for CN, so add the tableDesc in the LogicalScan.TableDesc
add all columnsLogicalSeqScan
LogicalValues
instead ofLogicalSeqScan
for some unit test. BTW, we will use optimizer: add mock plan node just for test #1253 to do the optimizer unit testLogicalSeqScan
andStreamScan
Checklist
Refer to a related PR or issue link (optional)
Exactly the same as #1250