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

fix(firebolt-schema-builder): better fallback to projection type resolution #1800

Merged

Conversation

ptiurin
Copy link
Contributor

@ptiurin ptiurin commented Dec 2, 2024

Description:

Previously a fresh setup of S3 source -> Firebolt sink would lead to an error by default due to the internal _meta/flow_truncated field's type not being resolved.
In this PR I'm trying to ensure we try to resolve the type via the projection whenever we're not able to do so with the explicit mapping from the Shape object.

Workflow steps:

Now setting up an S3 source and Firebolt sink should work by default.

Documentation links affected:

N/A

Notes for reviewers:

Added relevant tests to illustrate and test the issue.


This change is Reviewable

Copy link
Member

@jgraettinger jgraettinger left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks! FYI, inner array types are being included in the Inference message by this PR, which AFAIK will remove the need for this connector to use schemalate / extract directly from the JSON schema. This is a fine stop-gap.

@ptiurin
Copy link
Contributor Author

ptiurin commented Dec 3, 2024

FYI, inner array types are being included in the Inference message by this PR, which AFAIK will remove the need for this connector to use schemalate / extract directly from the JSON schema. This is a fine stop-gap.

Thanks for the heads-up! It would be great to have this connector more in line with the others! Though I'd imagine it would take some effort to refactor out schemalate so I think merging this PR in the meantime would let us proceed faster.

@jgraettinger jgraettinger merged commit b88f891 into estuary:master Dec 4, 2024
2 of 3 checks passed
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