-
Notifications
You must be signed in to change notification settings - Fork 15
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
bug: allow floats as timestamp column #777
Conversation
} else { | ||
Ok(time.clone()) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of this is halfway duplicated from the prepare_batch
code. It's not a complete duplicate, but there's definitely room to simplify, but it would require more refactoring of the column_behavior
code. Not sure how much we want to do now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The best options seem to be either:
- Drop the column behavior stuff and have both paths call
prepare_batch
(the newer code). - Add a column behavior corresponding to
numeric_to_timestamp
whichprepare_batch
uses for the timestamp case.
Either would let us avoid adding as much code to column_behavior
.
@@ -24,8 +25,15 @@ pub enum ColumnBehavior { | |||
/// Cast the given column to the given data type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to use / change the column behavior? I think we could just use the prepare_batch
method and avoid that. Alternatively, it seems like we could have a case here that called into the numeric_to_timestamp
and at least shared that case (rather than adding more complexity to column behaviors).
} else { | ||
Ok(time.clone()) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The best options seem to be either:
- Drop the column behavior stuff and have both paths call
prepare_batch
(the newer code). - Add a column behavior corresponding to
numeric_to_timestamp
whichprepare_batch
uses for the timestamp case.
Either would let us avoid adding as much code to column_behavior
.
) | ||
golden.jsonl(source) | ||
|
||
async def test_time_column_as_float_can_cast_ns(golden) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
Allows
timestamp
column to befloat
for parquet sources in the new prepare path.Also fixes the
time_unit
, which was previously not being taken into account.Closes #776