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

change started_at ended_at to be timestamps #53

Merged
merged 3 commits into from
Jan 19, 2024

Conversation

bakerfranke
Copy link
Contributor

Description

This PR casts the started_at and ended_at fields in int_school_years to be timestamps.

Directly from the seed file the were loaded as type varchar. This causes our use of these fields using date logic to incorrectly evaluate expressions using between because it's evaluating the dates as strings. What's moderately crazy is that this only effects exact date matches. For example:

SELECT
*
FROM int_school_years
WHERE '2023-07-01' between started_at and ended_at;

Returns null. But...(change the date July 2)....

SELECT
*
FROM int_school_years
WHERE '2023-07-02' between started_at and ended_at;

returns the expected row.

This can all be solved/fixed by casting started_at and ended_at as dates/timestamps.

The original analysis.school_years used timestamps so we will here as well.

Copy link
Collaborator

@allison-code-dot-org allison-code-dot-org left a comment

Choose a reason for hiding this comment

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

didn't realize these weren't timestamps already- good catch!

with
school_years as (
select *
select
school_year::varchar,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
school_year::varchar,
school_year::varchar(7),

school_year_int::integer,
started_at::timestamp,
ended_at::timestamp,
school_year_long::varchar
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
school_year_long::varchar
school_year_long::varchar(9)

Copy link
Collaborator

@jordan-springer jordan-springer left a comment

Choose a reason for hiding this comment

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

LGTM-- my only thought is to maintain precision here for school_year and school_year_long and to make sure we test the timestamp changes resolves what was called out above and doesn't adversely affect anything upstream in dbt

@jordan-springer jordan-springer self-assigned this Jan 17, 2024
@jordan-springer
Copy link
Collaborator

Description

This PR casts the started_at and ended_at fields in int_school_years to be timestamps.

Directly from the seed file the were loaded as type varchar. This causes our use of these fields using date logic to incorrectly evaluate expressions using between because it's evaluating the dates as strings. What's moderately crazy is that this only effects exact date matches. For example:

SELECT
*
FROM int_school_years
WHERE '2023-07-01' between started_at and ended_at;

Returns null. @bakerfranke but this should return null?...
But...(change the date July 2)....

SELECT
*
FROM int_school_years
WHERE '2023-07-02' between started_at and ended_at;

returns the expected row.

This can all be solved/fixed by casting started_at and ended_at as dates/timestamps.

The original analysis.school_years used timestamps so we will here as well.

Copy link
Collaborator

@jordan-springer jordan-springer left a comment

Choose a reason for hiding this comment

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

let's huddle about this date/timestamps thiing but this is fine.

@jordan-springer jordan-springer merged commit 7a3d15e into main Jan 19, 2024
@jordan-springer jordan-springer deleted the fix/int_school_years_timestamps branch January 19, 2024 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants