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 74.71% 74.68% -0.03%
==========================================
Files 43 43
Lines 8361 8525 +164
Branches 8361 8525 +164
==========================================
+ Hits 6247 6367 +120
- Misses 1727 1746 +19
- Partials 387 412 +25 ☔ 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 <scovich@users.noreply.github.com>
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.