-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add support for missing value interpretation #428
feat: add support for missing value interpretation #428
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this handwritten allow MVI changes when writes have already begun, or are the options locked at the outset only?
const protoDescriptor: DescriptorProto = | ||
adapt.convertStorageSchemaToProto2Descriptor(storageSchema, 'root'); | ||
|
||
// Row 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These sample rows are confusing, as the row data isn't compatible with the updatedSchema
above. If you're wanting empty rows that's fine, just describe what you're doing so the interaction is clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed some updates to the test to make the full schema more explicit. The idea here is to send rows with some empty values and show that they are being filled or not accordingly to the MVI settings.
Both |
src/managedwriter/json_writer.ts
Outdated
@@ -88,6 +87,30 @@ export class JSONWriter { | |||
this._encoder.setProtoDescriptor(protoDescriptor); | |||
} | |||
|
|||
/** | |||
* Update how missing values are interpreted by for the given stream. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the 'by' is extra in there.
src/managedwriter/writer.ts
Outdated
protoDescriptor: IDescriptorProto; | ||
|
||
/** | ||
* Controls how missing values are interpreted by for a given stream. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has the extra 'by' too.
src/managedwriter/writer.ts
Outdated
@@ -72,6 +126,28 @@ export class Writer { | |||
} | |||
} | |||
|
|||
/** | |||
* Update how missing values are interpreted by for the given stream. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another 'by'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates, comment nits seem to be the last bit to address.
I do wish the assertion support was better here and isnull/notnull were first class assertions, but alas.
src/managedwriter/writer.ts
Outdated
|
||
/** | ||
* Controls how missing values are interpreted by for a given stream. | ||
* `missingValueInterpretations` set for individual colums can override the default chosen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/colums/columns/
Fixes #426 🦕