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

case insensitive check on partition matching #888

Merged
merged 2 commits into from
Aug 22, 2023

Conversation

github-christophe-oudar
Copy link
Contributor

resolves #886

Problem

The API returns sometimes upper cased fields such timePartitioning.type and in case the user config has also a different casing, we should do a case insensitive check on partition matching.

Solution

It introduce a proper case insensitive check on partition field matching

Checklist

  • I have read the contributing guide and understand what's expected of me
  • 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
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX

@cla-bot cla-bot bot added the cla:yes label Aug 18, 2023
@github-christophe-oudar github-christophe-oudar marked this pull request as ready for review August 18, 2023 19:47
@github-christophe-oudar github-christophe-oudar requested a review from a team as a code owner August 18, 2023 19:47
or (conf_partition.time_ingestion_partitioning and table_field is not None)
) and table_granularity == conf_partition.granularity
) and table_granularity.lower() == conf_partition.granularity.lower()
Copy link
Contributor

Choose a reason for hiding this comment

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

Op's table actually didn't have the granularity key set:

-- models/foo_sans_granularity.sql
{{
  config(
    partition_by={
      "field": "updated_at",
      "data_type": "date",
    }
  )
}}

select id, updated_at, updated_at_utc from sources.raw

Do we need to account for that?

Choose a reason for hiding this comment

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

Daily partitioning is the default for all column types.

My understanding from the dbt docs was that the default granularity is day-partitioned.

Is this where the default is set?
https://github.com/dbt-labs/dbt-bigquery/blob/main/dbt/adapters/bigquery/impl.py#L77

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, there is a default so it should be safe

@dbeatty10 dbeatty10 added the ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering label Aug 18, 2023
@github-christophe-oudar
Copy link
Contributor Author

Thanks for the triage @dbeatty10 ! Could you enable the OK to test to run the ITs too?

@mikealfare
Copy link
Contributor

Re-opening to make the ok to test label trigger the tests

@mikealfare mikealfare closed this Aug 19, 2023
@mikealfare mikealfare reopened this Aug 19, 2023
@github-christophe-oudar
Copy link
Contributor Author

@mikealfare it looks like both this PR & #866 are failing over tests/functional/adapter/test_basic.py
I couldn't reproduce the issue locally so I wonder if there's an issue with CI?

@McKnight-42
Copy link
Contributor

@github-christophe-oudar rerunning tests to see if it was ci/cd.

@github-christophe-oudar
Copy link
Contributor Author

It's still failing on those tests and I don't know why 🤔

@McKnight-42 McKnight-42 merged commit 2c7220a into dbt-labs:main Aug 22, 2023
22 checks passed
@McKnight-42
Copy link
Contributor

@github-christophe-oudar great work on this.

McKnight-42 pushed a commit that referenced this pull request Aug 22, 2023
* case insensitive check on partition matching

* Review change

---------

Co-authored-by: Christophe Oudar <[email protected]>
@github-christophe-oudar
Copy link
Contributor Author

Great! Thanks for the swift turnaround on this bugfix 🙌
Looking forward for the next minor release so that anyone can benefit from it 👍

nathaniel-may pushed a commit that referenced this pull request Aug 23, 2023
* case insensitive check on partition matching

* Review change

---------

Co-authored-by: Christophe Oudar <[email protected]>
Co-authored-by: Christophe Oudar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes ok to test ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ADAP-834] [Regression] adapter.is_replacable() always evaluates to False with dbt-bigquery 1.6
7 participants