[GIE MetaData] Support MetaData for IR Algebra/Physical Operators #2480
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2480 +/- ##
===========================================
- Coverage 72.62% 39.87% -32.76%
===========================================
Files 88 88
Lines 9816 9816
===========================================
- Hits 7129 3914 -3215
- Misses 2687 5902 +3215 see 47 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| expr.operators.get(0) | ||
| if let Some(common_pb::ExprOpr { | ||
| item: Some(common_pb::expr_opr::Item::Var(var)), .. | ||
| }) = expr.operators.get(0) |
There was a problem hiding this comment.
use first() instead of get(0)
| @@ -209,6 +209,15 @@ impl Plan { | |||
| self | |||
| } | |||
|
|
|||
| // Notice that this is used to set the meta_data of the **Last Appended OP** | |||
| pub fn with_meta_data(&mut self, meta_data: Option<Vec<pb::physical_opr::MetaData>>) { | |||
There was a problem hiding this comment.
Why Vec requires Option? Empty Vec stands for None?
| pub fn with_meta_data(&mut self, meta_data: Option<Vec<pb::physical_opr::MetaData>>) { | ||
| if let Some(op) = self.plan.last_mut() { | ||
| if let Some(meta_data) = meta_data { | ||
| op.op_meta = meta_data; |
There was a problem hiding this comment.
Probably meta_data is a better name than op_meta.
| @@ -41,6 +47,8 @@ message Project { | |||
| // An indicator to tell the runtime whether the projected value is appending to or replacing | |||
| // existing relation. | |||
| bool is_append = 2; | |||
| // The datatype of output results | |||
| repeated MetaData op_meta = 3; | |||
There was a problem hiding this comment.
Why we donot use map<alias, IrDataType> for MetaData? As before, think meta_data is a better name than op_meta
There was a problem hiding this comment.
Have renamed as meta_data. repeated MetaData here is defined in consist with, e.g., repeated ExprAlias in Project.
| @@ -153,6 +153,7 @@ impl From<IrError> for FfiResult { | |||
| IrError::InvalidPattern(s) => FfiResult::new(ResultCode::Others, s), | |||
| IrError::InvalidExtendPattern(err) => FfiResult::new(ResultCode::Others, err.to_string()), | |||
| IrError::PbEncodeError(err) => FfiResult::new(ResultCode::Others, err.to_string()), | |||
| IrError::PbDecodeError(err) => FfiResult::new(ResultCode::Others, err.to_string()), | |||
There was a problem hiding this comment.
Modifed as ResultCode::ParsePbError here.
| @@ -800,6 +848,29 @@ fn set_predicate(ptr: *const c_void, cstr_predicate: *const c_char, opt: InnerOp | |||
| } | |||
| } | |||
|
|
|||
| /// To set an operator's predicate from a pb predicate pointer | |||
| fn set_predicate_with_pb(ptr: *const c_void, ptr_predicate_pb: FfiPbPointer, opt: InnerOpt) -> FfiResult { | |||
There was a problem hiding this comment.
This is set_predicate_pb, rather than set_predicate_with_pb. Same apply to a few following functions. Also comment this function: In the following functions, we can set expression directly from a pb pointer, which is parsed by the compiler instead of ffi function.
| @@ -895,6 +966,13 @@ mod params { | |||
| set_predicate(ptr_params, cstr_pred, InnerOpt::Params) | |||
| } | |||
|
|
|||
| #[no_mangle] | |||
| pub extern "C" fn set_params_predicate_with_pb( | |||
There was a problem hiding this comment.
Same set_params_predicate_pb
| /// To add a mapping for the project operator, which maps a pb pointer to represent an | ||
| /// expression, and a `NameOrId` parameter that represents an alias. | ||
| #[no_mangle] | ||
| pub extern "C" fn add_project_expr_alias_with_pb( |
There was a problem hiding this comment.
add_project_expr_pb_alias
| self.plan.unfold(unfold.into()).with_meta_data( | ||
| meta_data | ||
| .map(|meta| vec![meta.into()]) | ||
| .unwrap_or(vec![]), |
There was a problem hiding this comment.
Could use unwrap_or_default()
What do these changes do?
Related issue number
#2417