Fix partitioning by DATE column #1057
Conversation
product-auto-label
bot
added
size: s
product-auto-label
bot
added
size: s
| if time_partitioning.field is not None: | ||
| field = time_partitioning.field | ||
| if isinstance( | ||
| table.columns[time_partitioning.field].type, | ||
| sqlalchemy.sql.sqltypes.DATE, | ||
| ): | ||
| return f"PARTITION BY {field}" | ||
| elif isinstance( | ||
| table.columns[time_partitioning.field].type, | ||
| sqlalchemy.sql.sqltypes.TIMESTAMP, |
There was a problem hiding this comment.
@tswast this PR is looking at adding logic to also handle partitioning on a date above and beyond using just DATE_TRUNC and TIMESTAMP_TRUNC functions.
Looking at the breakdown :TIMESTAMP_TRUNC: and :DATE_TRUNC: of how bigquery accepts time partitioning, it feels like we are missing a variety of the options/possibilities.
Am I missing something?
There was a problem hiding this comment.
@chalmerlowe can you clarify what you think is missing? it looks like the API expects one of
so it seems like TIMESTAMP_TRUNC would work for all of those and DATE would work for MONTH and YEAR, so all that's missing is just plain DATE
There was a problem hiding this comment.
| Column Data Type | HOUR | DAY | MONTH | YEAR |
|---|---|---|---|---|
| DATE | N/A | Fixed by #1057 | via DATE_TRUNC | via DATE_TRUNC |
| DATETIME | Currently via DATE_TRUNC TODO: use DATETIME_TRUNC | Currently via DATE_TRUNC TODO: use DATETIME_TRUNC | Currently via DATE_TRUNC TODO: use DATETIME_TRUNC | Currently 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 supported TODO: USE TIMESTAMP_TRUNC | Not supported TODO: USE TIMESTAMP_TRUNC | Not supported TODO: USE TIMESTAMP_TRUNC | Not supported TODO: USE TIMESTAMP_TRUNC |
There was a problem hiding this comment.
@bnaul For clarity, my question was less about whether this PR is correct and more focused on the bigger question:
Since we have the function _process_time_partitioning() and it handles two use cases (soon to be three based on this PR), does it cover all the use cases that it ought to?
Tim and I looked at this. I had a matrix to help me ID what the function does/doesn't do/should do.
Tim cleaned it up and posted it above. There are capabilities that are satisfied via an existing BigQuery function (i.e. DATE_TRUNC). But there are use cases that are not fully satisfied OR not supported at all. Tim provides suggestions for how to resolve those gaps.
NOTE: In no way am I saying you need to do all this work. This is more for me as a maintainer to figure out what I want the end state to be either through this PR or a follow-on PR.
There was a problem hiding this comment.
ah yes that matrix is very helpful, thanks!
There was a problem hiding this comment.
Just curious @chalmerlowe @tswast, any more thoughts on whether this PR should go ahead or wait on additional changes? To me it's distinct enough from the other "Not supported" cases above that it makes sense to keep them separate, but obviously I'm a bit biased 🙂
chalmerlowe
added
kokoro:run
yoshi-kokoro
removed
kokoro:run
|
The reason the Kokoro check is failing is because we do not detect full test coverage. Because the test we have included here is in the I recommend adding a test to the |
Fixes #1056