Skip to content

feat(schema): Add schema.TypeTimeInterval#205

Merged
candiduslynx merged 3 commits intomainfrom
feat/duration-type
Sep 30, 2022
Merged

feat(schema): Add schema.TypeTimeInterval#205
candiduslynx merged 3 commits intomainfrom
feat/duration-type

Conversation

@candiduslynx
Copy link
Copy Markdown
Contributor

@candiduslynx candiduslynx commented Sep 30, 2022

Summary

Add duration column type for #178


Use the following steps to ensure your PR is ready to be reviewed

  • Read the contribution guidelines 🧑‍🎓
  • Run go fmt to format your code 🖊
  • Lint your changes via golangci-lint run 🚨 (install golangci-lint here)
  • Update or add tests 🧪
  • Ensure the status checks below are successful ✅

@candiduslynx candiduslynx self-assigned this Sep 30, 2022
@candiduslynx candiduslynx linked an issue Sep 30, 2022 that may be closed by this pull request
@github-actions github-actions bot added feat and removed feat labels Sep 30, 2022
@candiduslynx candiduslynx enabled auto-merge (squash) September 30, 2022 11:53
@candiduslynx candiduslynx enabled auto-merge (squash) September 30, 2022 11:53
Copy link
Copy Markdown
Contributor

@yevgenypats yevgenypats left a comment

Choose a reason for hiding this comment

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

Looks good. One question - maybe we should call it interval? i.e stick to postgresql types? or at least be consistent with one db? as in the json when it is being sent over the wire it is string anyway.

Copy link
Copy Markdown
Contributor

@hermanschaaf hermanschaaf left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Let's mark it as a breaking change though, because destination plugins will need to explicitly handle this new type.

@yevgenypats
Copy link
Copy Markdown
Contributor

LGTM 👍

Let's mark it as a breaking change though, because destination plugins will need to explicitly handle this new type.

I think for destination plugin we need to always default to text as this field can contain anything and we shouldn't break destinations every-time we support new field in CQ.

So I wouldn't mark it as breaking even it will break:) maybe just follow-up PR with the right behaviour to default to text?

@candiduslynx
Copy link
Copy Markdown
Contributor Author

Now it'll be treated as int64 -> schema.TypeInt, so IDK if that's a good idea to convert this to text.

As for the interval - will do

@candiduslynx candiduslynx changed the title feat(schema): Add schema.TypeDuration feat(schema): Add schema.TypeTimeInterval Sep 30, 2022
@github-actions github-actions bot added feat and removed feat labels Sep 30, 2022
@candiduslynx candiduslynx merged commit 02fce2c into main Sep 30, 2022
@candiduslynx candiduslynx deleted the feat/duration-type branch September 30, 2022 14:52
candiduslynx pushed a commit that referenced this pull request Sep 30, 2022
🤖 I have created a release *beep* *boop*
---


##
[0.11.3](v0.11.2...v0.11.3)
(2022-09-30)


### Features

* **schema:** Add schema.TypeDuration
([#205](#205))
([02fce2c](02fce2c))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add column type for time interval (duration)

3 participants