Map access for expressions #352
Conversation
…ion evaluation on that map access
…ion evaluation on that map access
hntd187
requested review from
nicklan,
scovich,
zachschuermann,
roeap and
OussamaSaoudi-db
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #352 +/- ##
==========================================
- Coverage 77.99% 77.65% -0.34%
==========================================
Files 49 49
Lines 10328 10535 +207
Branches 10328 10535 +207
==========================================
+ Hits 8055 8181 +126
- Misses 1821 1876 +55
- Partials 452 478 +26 ☔ View full report in Codecov by Sentry. |
kernel/src/actions/schemas.rs
Outdated
| } | ||
|
|
||
| fn nullable() -> bool { | ||
| V::nullable() |
There was a problem hiding this comment.
This doesn't look right? The nullability of a list (or map) should be independent of whether the list elements (mapped values) are nullable? ie Option<Vec<T>> and Vec<Option<T>> (or Option<HashMap> and HashMap<K, Option<V>>) should be orthogonal?
There was a problem hiding this comment.
I ended up removing this based on Nick's feedback
kernel/src/engine/arrow_get_data.rs
Outdated
| let values = map_struct.column(1).as_string::<i32>(); | ||
| for (key, value) in keys.iter().zip(values.iter()) { | ||
| if let (Some(key), value) = (key, value) { | ||
| ret.insert(key.into(), value.map(Into::into)); |
There was a problem hiding this comment.
It looks like this is the only line that differs between the two methods; is there a way to factor it out with a lambda arg to capture the value transformation?
There was a problem hiding this comment.
This was also removed because it wasn't necessary and partitionValues when back to being HashMap<String, String>
Co-authored-by: Ryan Johnson <[email protected]>
…l-rs into map_access # Conflicts: # Cargo.toml # kernel/src/engine_data.rs
github-actions
bot
added
the
breaking-change
| @@ -318,27 +363,21 @@ fn evaluate_expression( | |||
| .map(wrap_comparison_result) | |||
| .map_err(Error::generic_err) | |||
| } | |||
| (BinaryOperation { op, left, right }, _) | |||
| if matches!(**left, MapAccess { .. }) && matches!(**right, Literal(_)) => | |||
There was a problem hiding this comment.
Why is it important that we handle BinaryOperation with left: MapAccess, and right: Literal? What about the other way around with left: Literal, right: MapAccess?
There was a problem hiding this comment.
Conceptually, we're doing map['key']; 'key'[map] doesn't really make sense?
There was a problem hiding this comment.
Well the left side is map['key'] by itself, so if you knew the key you'd write map['key'] = 1 or whatever so the map access left side essentially boils down to a literal vs literal match.
These support general map access, access of specific keys and binary expression against maps. These expressions have some null semantics which need to be discussed and agreed upon.
I removed the
HashMap<_, Option<String>>on partition values since Nick had already done something for it. This is a draft for now I'll add more context for discussion.