feat(csv): treat omitted values with no defaults as null in csv #691
Conversation
| panic(fmt.Errorf("invalid value: %s", t.KeyValues[j])) | ||
| var v values.Value | ||
| if t.KeyValues[j] == nil { | ||
| v = values.NewNull(flux.SemanticType(t.ColMeta[idx].Type)) |
There was a problem hiding this comment.
Is there a way to make it so values.New(nil) works instead of adding a new function?
There was a problem hiding this comment.
I thought of that too, but a values.Value has to have a type.
There was a problem hiding this comment.
We want null values to have a type, so I don't see a way to do it just by passing nil. If New accepted a pointer to a value, then it would be possible by passing a null pointer... but that seems kind of hacky to me.
csv/result.go
Outdated
| @@ -397,6 +397,8 @@ func readMetadata(r *csv.Reader, c ResultDecoderConfig, extraLine []string) (tab | |||
| return tableMetadata{}, errors.Wrapf(err, "column %q has invalid default value", label) | |||
| } | |||
| defaultValues[j] = v | |||
| } else { | |||
| defaultValues[j] = values.NewNull(flux.SemanticType(cols[j].ColMeta.Type)) | |||
There was a problem hiding this comment.
Can you add a constant for the nullValue and set it to "" for now? We want to make it clear how the null value interacts with the default value. The if statement above it would become switch statement with three cases:
switch defaults[j] {
case nullValue:
// Set explicit null as default
defaultValues[j] = values.NewNull(flux.SemanticType(cols[j].ColMeta.Type))
case "":
// Do nothing there is no default
default:
v, err := decodeValue(defaults[j], cols[j])
// ...
}Technically the second case will not be possible until nullValue is dynamic. But it makes it clear that its still possible to not have a default value.
There was a problem hiding this comment.
I can't write the code like you've done it here because Go complains about duplicate cases, but I will add nullValue and comments to this effect.
csv/result.go
Outdated
| @@ -1104,23 +1104,38 @@ func encodeValue(value values.Value, c colMeta) (string, error) { | |||
| } | |||
| } | |||
|
|
|||
| func encodeValueFrom(i, j int, c colMeta, cr flux.ColReader) (string, error) { | |||
| func encodeValueFrom(i, j int, c colMeta, cr flux.ArrowColReader) (string, error) { | |||
| var v string | |||
There was a problem hiding this comment.
We should use the nullValue constant here too
| @@ -1192,6 +1192,10 @@ func (b *ColListTableBuilder) SetValue(i, j int, v values.Value) error { | |||
| } | |||
|
|
|||
| func (b *ColListTableBuilder) AppendValue(j int, v values.Value) error { | |||
| if v.IsNull() { | |||
There was a problem hiding this comment.
This is so useful and safety, I want to propose making AppendBool, AppendInt, etc. all private functions, and forcing us to use AppendValue(j, values.New(false)), etc.
There was a problem hiding this comment.
I suspect we'll still want to expose the methods that accept raw types for performance reasons, since values.Value uses interface{} and reflection under the hood. Were it not for this I would totally agree with you.
| @@ -148,6 +152,13 @@ func New(v interface{}) Value { | |||
| } | |||
| } | |||
|
|
|||
| func NewNull(t semantic.Type) Value { | |||
There was a problem hiding this comment.
this is great. I'm getting closer to voting that we can only add values.Value types to table builders, to make sure we don't have any errors of omission re: Nulls.
wolffcm
force-pushed
the
feat/csv-nulls
branch
from
b6db038 to
9caa2bc
Compare
Fixes #632.
Done checklist