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.

Sign up for GitHub

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

Jump to bottom

Add microbatch incremental strategy #453

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add microbatch incremental strategy #453

wants to merge 1 commit into from
+62 −2

Conversation

damian3031
Copy link
Member

Overview

Add microbatch incremental strategy.
background:
dbt-labs/dbt-core#10672

This will be released in dbt-trino 1.9

Checklist

  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • README.md updated and added information about my change
  • I have run changie new to create a changelog entry

@damian3031 damian3031 requested review from aalbu and mdesmet November 29, 2024 20:08
@github-actions GitHub Actions
Copy link

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@damian3031 damian3031 force-pushed the damian/microbatch-strategy branch from 42c00de to c5146c5 Compare December 2, 2024 12:28
mdesmet
mdesmet approved these changes Dec 9, 2024
View reviewed changes
Copy link
Member

@mdesmet mdesmet left a comment

Choose a reason for hiding this comment

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

LGTM (some stuff for thinking related to timezones but I think it is good to go)

tests/functional/adapter/materialization/test_incremental_microbatch.py


@pytest.mark.iceberg
class TestTrinoMicrobatchIceberg(BaseMicrobatch):
Copy link
Member

Choose a reason for hiding this comment

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

What types are tested? I guess all should work using the TIMESTAMP 'syntax'. Timezones are out of scope according dbt documentation. I assume that the type of the field needs to TIMESTAMP WITH TIMEZONE?

Copy link
Member

Choose a reason for hiding this comment

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

Seems that the tests us e TIMESTAMP and not TIMESTAMP WITH TIMEZONE. See https://github.com/dbt-labs/dbt-core/blob/03fdb4c1578d74e25615a4f46fb572ecee0685e0/tests/functional/microbatch/test_microbatch.py#L174

I guess this is OK as most of the time the Trino server will be configured in UTC timezone and event times will be in UTC.

@damian3031 damian3031 force-pushed the damian/microbatch-strategy branch from c5146c5 to a0de922 Compare December 9, 2024 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdesmet mdesmet mdesmet approved these changes

@aalbu aalbu Awaiting requested review from aalbu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@damian3031 @mdesmet