[1st version] Add Tuple idl #5720
Conversation
Signed-off-by: Mecoli1219 <[email protected]>
Signed-off-by: Mecoli1219 <[email protected]>
Signed-off-by: Mecoli1219 <[email protected]>
Signed-off-by: Mecoli1219 <[email protected]>
Signed-off-by: Mecoli1219 <[email protected]>
| case *core.LiteralType_TupleType: | ||
| // TO_DISCUSS: How to handle the tuple interface in Inputs field of flytectl? | ||
| // | ||
| // [Example usage] | ||
| // inputs: | ||
| // tuple_data: | ||
| // key1: "foo" | ||
| // key2: 123 | ||
|
|
||
| vMap, ok := v.(map[string]interface{}) | ||
| if !ok { | ||
| return nil, errors.Errorf("Expected a map[string]interface{} for tuple type, got [%v]", v) |
There was a problem hiding this comment.
In this part, the LiteralType is already given, so the name of the tuple and the order of each field should be already defined. The only thing that is missing is the value of each field.
Therefore, we only need to provide the value of each tuple field (via the key-value pair of each field) in flytectl for the registered task or workflow for further execution needs.
Signed-off-by: Mecoli1219 <[email protected]>
Signed-off-by: Mecoli1219 <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5720 +/- ##
===========================================
+ Coverage 36.30% 51.00% +14.70%
===========================================
Files 1305 1159 -146
Lines 110000 75156 -34844
===========================================
- Hits 39932 38332 -1600
+ Misses 65912 32845 -33067
+ Partials 4156 3979 -177
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| repeated string order = 2; | ||
|
|
||
| // A map of literals. | ||
| map<string, Literal> literals = 3; |
There was a problem hiding this comment.
Can we use Literal.collection?
There was a problem hiding this comment.
Literal.collection is not enough to store the name of the fields for NamedTuple.
| message TupleType { | ||
| string tuple_name = 1; | ||
| repeated string order = 2; | ||
| map<string, LiteralType> fields = 3; |
There was a problem hiding this comment.
Can we use repeated here, like
message TupleEntry {
string name = 1;
LiteralType literal_type = 2;
}
message TupleType {
string tuple_name = 1;
repeated TupleEntry = 2;
}There was a problem hiding this comment.
Other Considerations
An alternative approach to storing the literal values for NamedTuple and Tuple would be combining the order and the map into a list of messages, as shown below:
// !!! This is not the proposed implementation !!!
message FakeLiteralTupleField {
string name = 1;
Literal value = 2;
}
message FakeLiteralTupleMap {
string tuple_name = 1;
repeated FakeLiteralTupleField fields = 2;
}
While this structure may seem more straightforward, it has a drawback: looking up a field’s value by name requires iterating through the list, which results in O(n) complexity. In contrast, using a map allows for more efficient O(1) lookup times when retrieving the value of a field by its name. Although in practice the tuple size is usually small, the map-based approach is more scalable and efficient.
Tracking issue
#5908
#1337
#3158
#4358
Why are the changes needed?
What changes were proposed in this pull request?
How was this patch tested?
Setup process
Screenshots
Check all the applicable boxes
Related PRs
RFC: #5699
Docs link