-
Notifications
You must be signed in to change notification settings - Fork 179
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: Add DataType inference from Python types #3555
Conversation
CodSpeed Performance ReportMerging #3555 will degrade performances by 12.53%Comparing Summary
Benchmarks breakdown
|
@pytest.mark.parametrize( | ||
["source", "expected"], | ||
[ | ||
(str, DataType.string()), |
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.
perhaps could test
dict
(ourDataType::Map
)tuple
(ourDataType::Struct
I think with just arbitrary names)bytearray
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.
trust your judgement here though :)
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.
Oh yes the dict map one is a good idea. Let me add that.
daft/udf.py
Outdated
@@ -394,7 +394,7 @@ def __hash__(self) -> int: | |||
|
|||
def udf( | |||
*, | |||
return_dtype: DataType, | |||
return_dtype: DataType | 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.
I wonder if we could create a type alias in pythong for DataType | type
as it seems to be used in a lot of places
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.
I remember documentation being a bit messy when I used type aliases for something else. Let me give it a shot again and see how it shows up on docs.
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.
ah I forgot how it shows up in documentation... yea I guess I am thinking like a Rust user. it might be complicated for something that isn't statically typed having to go deeper in docs 🤔. Trust your judgement here.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3555 +/- ##
==========================================
+ Coverage 77.69% 77.79% +0.09%
==========================================
Files 710 716 +6
Lines 86941 87801 +860
==========================================
+ Hits 67552 68305 +753
- Misses 19389 19496 +107
|
Closes #3549