Skip to content
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

Allow all schemas to fall back to a string value #25

Merged
merged 4 commits into from
Feb 18, 2021

Conversation

luandy64
Copy link
Contributor

Description of change

This PR changes discovery to write an anyOf schema for all fields.

Manual QA steps

  • Made a sheet where a column gets a date-time schema, but some row after the first has a string instead of a date
  • Made a sheet where a column gets a time schema
    • We currently output times like "123 days, 12:34:56"
  • Made a sheet where a column gets a date-time schema, but some row after the first has a null cell

On master, the tap fails with transform errors for the first two test cases. On this branch, there are no transform errors and the records emitted have string values as expected.

On this branch, for the null cell test case, no transform error occurs and a null is emitted as expected.

Risks

  • For tap runs that were previously encountering a transform error, this PR will likely cause a column split in downstream systems.
  • For tap runs that were succeeding before, there should be no change.

Rollback steps

  • revert this branch and bump the version

Copy link
Contributor

@KAllan357 KAllan357 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the anyOf should be on the specific date/time schema portions instead of applied to all. When its applied to them all, you'd end up with funny schema like anyOf: [null string], [null, string]

@luandy64 luandy64 merged commit 80f9752 into master Feb 18, 2021
@luandy64 luandy64 deleted the string-schema-fallback branch February 18, 2021 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants