Preserve list element name #2840
Conversation
# Conflicts: # datafusion/proto/src/from_proto.rs
github-actions
bot
added
core
Codecov Report
@@ Coverage Diff @@
## master #2840 +/- ##
==========================================
- Coverage 85.19% 85.18% -0.02%
==========================================
Files 275 275
Lines 48764 48756 -8
==========================================
- Hits 41546 41534 -12
- Misses 7218 7222 +4
Continue to review full report at Codecov.
|
datafusion/proto/src/from_proto.rs
Outdated
| @@ -656,21 +658,22 @@ impl TryFrom<&protobuf::ScalarListValue> for ScalarValue { | |||
| fn try_from( | |||
| scalar_list_value: &protobuf::ScalarListValue, | |||
| ) -> Result<Self, Self::Error> { | |||
| use protobuf::{scalar_type::Datatype, PrimitiveScalarType}; | |||
| if 2 > 1 { | |||
There was a problem hiding this comment.
@tustvold im not sure why all tests are ok, looks like this piece of code not used. The CI build has Ballista tests?
There was a problem hiding this comment.
Maybe it is because the tests don't have coverage of DataType::List
This is a good experiment (add in a panic) but we probably need to remove this prior to merging this PR
There was a problem hiding this comment.
Right,I added this to check if this code part is used. We have roundtrips tests for lists and nested lists in round_trip_scalar_values which passed. I'm wondering if ballista tests are in CI? Otherwise this code looks useless and can be dropped
There was a problem hiding this comment.
I think the ballista CI checks only run in the ballista repo 🤔
| ScalarValue::List(value, Box::new(data_type)) | ||
| ScalarValue::List( | ||
| value, | ||
| Box::new(Field::new("item", nested_type.data_type().clone(), true)), |
There was a problem hiding this comment.
this is effectively going to lose the original field name (which isn't attached to the array) in this particular codepath, which is a similar problem to what I observed in #2874
However, this PR certainly seems better than the current state of master which loses the field name through all paths
datafusion/proto/src/from_proto.rs
Outdated
| @@ -656,21 +658,22 @@ impl TryFrom<&protobuf::ScalarListValue> for ScalarValue { | |||
| fn try_from( | |||
| scalar_list_value: &protobuf::ScalarListValue, | |||
| ) -> Result<Self, Self::Error> { | |||
| use protobuf::{scalar_type::Datatype, PrimitiveScalarType}; | |||
| if 2 > 1 { | |||
There was a problem hiding this comment.
Maybe it is because the tests don't have coverage of DataType::List
This is a good experiment (add in a panic) but we probably need to remove this prior to merging this PR
|
Marking as a draft while we work out feedback; Please mark ready for review when it is ready again 🙏 |
Which issue does this PR close?
Closes #2450 .
Rationale for this change
What changes are included in this PR?
Preserve list element name
Are there any user-facing changes?