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

Feat: Update partitioning by DATE, DATETIME, TIMESTAMP, _PARTITIONDATE #1113

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chalmerlowe
Copy link
Collaborator

This adds some additional functionality to properly handle partitioning of columns with the following datatypes.

  • DATE
  • TIMESTAMP
  • DATETIME
  • _PARTITIONDATE

Where appropriate, ensures the following functions can be used with/or without the following TimePartitioningTypes (HOUR, DAY, MONTH, YEAR).

  • DATE_TRUNC()
  • TIMESTAMP_TRUNC()
  • DATETIME_TRUNC()
  • DATE()

This is a nearly complete fix for #1072. The table from #1072 is included here:
NOTE: This PR does not handle _PARTITIONTIME

Column Data Type HOUR DAY MONTH YEAR
DATE N/A Fixed by #1057 via DATE_TRUNC via DATE_TRUNC
DATETIME Incorrectly implemented via DATE_TRUNC TODO: use  DATETIME_TRUNC Incorrectly implemented via DATE_TRUNC TODO: use  DATETIME_TRUNC Incorrectly implemented via DATE_TRUNC TODO: use  DATETIME_TRUNC Incorrectly implemented via DATE_TRUNC TODO: use  DATETIME_TRUNC
TIMESTAMP via TIMESTAMP_TRUNC via TIMESTAMP_TRUNC via TIMESTAMP_TRUNC via TIMESTAMP_TRUNC
_PARTITIONDATE N/A via DATE_TRUNC via DATE_TRUNC via DATE_TRUNC
_PARTITIONTIME Not currently implemented TODO: USE TIMESTAMP_TRUNC Not currently implemented TODO: USE TIMESTAMP_TRUNC Not currently implemented TODO: USE TIMESTAMP_TRUNC Not currently implemented TODO: USE TIMESTAMP_TRUNC

Copy link

conventional-commit-lint-gcf bot commented Sep 11, 2024

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: bigquery Issues related to the googleapis/python-bigquery-sqlalchemy API. labels Sep 11, 2024
field = "_PARTITIONDATE"
trunc_fn = "DATE_TRUNC"

# Format used with _PARTITIONDATE which can only be used for
# DAY / MONTH / YEAR
if time_partitioning.field is None and field == "_PARTITIONDATE":
Copy link

Choose a reason for hiding this comment

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

field == "_PARTITIONDATE" is always true

# DAY / MONTH / YEAR
if time_partitioning.field is None and field == "_PARTITIONDATE":
if time_partitioning.type_ in {"DAY", "MONTH", "YEAR"}:
return f"PARTITION BY {trunc_fn}({field})"
Copy link

Choose a reason for hiding this comment

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

I'm a little confused, the type isn't passed to the trunc_fn, should it be?

field = "_PARTITIONDATE"
trunc_fn = "DATE_TRUNC"

# Format used with _PARTITIONDATE which can only be used for
Copy link

Choose a reason for hiding this comment

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

There are a lot of cases here that make this function hard to parse. In particular, I think it could be improved if instead of the first special casing for time.partitioning.field is None block was instead combined by deleting that check, and moving the if time.partioning.type_ in ... below the if field is not None block:

        field = "_PARTITIONDATE"
        trunc_fn = "DATE_TRUNC"
        if time_partitioning.field is not None:
            field = time_partitioning.field

            if isinstance(
                table.columns[field].type,
                (sqlalchemy.sql.sqltypes.TIMESTAMP),
            ):
                trunc_fn = "TIMESTAMP_TRUNC"
            elif isinstance(
                table.columns[field].type,
                sqlalchemy.sql.sqltypes.DATETIME,
            ):
                  trunc_fn = "DATETIME_TRUNC"



        if trunc_fn == "DATE_TRUNC" and not time_partitioning.type_ in {"DAY", "MONTH", "YEAR"}:
               raise ValueError(
                      f"DATE_TRUNC can only be used with TimePartitioningTypes {{DAY, MONTH, YEAR}} received {time_partitioning.type_}"
                  )

        # Format used with generically with DATE, TIMESTAMP, DATETIME, DATE_TRUNC
        return f"PARTITION BY {trunc_fn}({field}, {time_partitioning.type_})"**

Copy link

Choose a reason for hiding this comment

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

I had removed the the last if isinstance(table.columns[field].type,, because I thought it was part of the if elif, I see now that its separate. If thats the case, it could be the first if condition so it can take priority instead of setting it back after the fact (pattern matching would be nice here when we support >= python 3.10)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the googleapis/python-bigquery-sqlalchemy API. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants