Skip to content

feat(postgresql): Add interval column type for schema.TimeInterval#2167

Merged
candiduslynx merged 4 commits intomainfrom
feat/time-interval
Oct 13, 2022
Merged

feat(postgresql): Add interval column type for schema.TimeInterval#2167
candiduslynx merged 4 commits intomainfrom
feat/time-interval

Conversation

@candiduslynx
Copy link
Copy Markdown
Contributor

Add interval column type for schema.TimeInterval (#2062)

@candiduslynx candiduslynx requested review from a team and cqgaurav and removed request for a team September 30, 2022 16:30
@candiduslynx candiduslynx linked an issue Sep 30, 2022 that may be closed by this pull request
@candiduslynx candiduslynx enabled auto-merge (squash) September 30, 2022 16:32
@candiduslynx candiduslynx self-assigned this Sep 30, 2022
@candiduslynx candiduslynx added enhancement automerge Automatically merge once required checks pass labels Sep 30, 2022
@candiduslynx candiduslynx requested review from hermanschaaf and yevgenypats and removed request for cqgaurav September 30, 2022 17:33
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. Can you also add a default for text and maybe also log a warning that it doesn't support a specific type.

@hermanschaaf
Copy link
Copy Markdown
Contributor

@yevgenypats I think we need to carefully consider whether to default to text here--it can result in changed behaviour whenever the SDK adds a new type. For example, intervals were previously stored as int, so if we defaulted to text without explicitly handling it in this plugin, then those would have silently started to convert to text. I'd rather have it fail very loudly when we upgrade the SDK to a version that added a new type, so it forces you to add support or fall back to the right thing.

Maybe a solution is to add a test that iterates over all types in the SDK, checking they are all handled? Then you wouldn't be able to upgrade the SDK to a version with a new type without also updating the handled types.

@candiduslynx candiduslynx force-pushed the feat/time-interval branch 2 times, most recently from 2b13de8 to 29a53ce Compare October 12, 2022 08:14
@candiduslynx candiduslynx force-pushed the feat/time-interval branch 4 times, most recently from ce5648b to 2ad5045 Compare October 12, 2022 10:09
Squashed commits:
[adacabd] add interval col impl
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, just wondering why we chose to rely on defaults for the cockroachdb case

@yevgenypats
Copy link
Copy Markdown
Contributor

@yevgenypats I think we need to carefully consider whether to default to text here--it can result in changed behaviour whenever the SDK adds a new type. For example, intervals were previously stored as int, so if we defaulted to text without explicitly handling it in this plugin, then those would have silently started to convert to text. I'd rather have it fail very loudly when we upgrade the SDK to a version that added a new type, so it forces you to add support or fall back to the right thing.

Maybe a solution is to add a test that iterates over all types in the SDK, checking they are all handled? Then you wouldn't be able to upgrade the SDK to a version with a new type without also updating the handled types.

Maybe we can have an option then. because let's say a community plugin is not up to date, it will be pretty hard requirement always staying up to date with all latest fields. maybe we can ignore this column as well? I think let's wait and see what issues this will cause.

@candiduslynx candiduslynx merged commit f2e6a53 into main Oct 13, 2022
@candiduslynx candiduslynx deleted the feat/time-interval branch October 13, 2022 07:07
kodiakhq bot pushed a commit that referenced this pull request Oct 13, 2022
🤖 I have created a release *beep* *boop*
---


## [1.3.0](plugins-destination-postgresql-v1.2.2...plugins-destination-postgresql-v1.3.0) (2022-10-13)


### Features

* **postgresql:** Add interval column type for `schema.TimeInterval` ([#2167](#2167)) ([f2e6a53](f2e6a53))

---
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

Labels

automerge Automatically merge once required checks pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add time interval to the destination

4 participants