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

add infer types parameter in DSL functions #579

Merged
merged 1 commit into from
Feb 13, 2024
Merged

Conversation

koperagen
Copy link
Collaborator

I believe tests that i added speak for themselves. Type can often be erased to Any and we need an easy way to get a meaningful result

fun `add several columns with type inference`() {
val f: Any = 123
val df = typed.add {
expr(infer = Infer.Type) { f } into "f"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually expected this to be done by default. If you need to manually specify to infer types, I think people will just not do it. And then, it will be Any?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's consistent with add itself (default infer = Infer.Nulls) and most other usages of this enum. I think it makes sense. Looking at values to deduce type is a reflective operation and, while i didn't test it myself and decision wasn't made by me, i suppose it can be quite heavy + produce unexpected results (more precise type than needed or something). So i'd say keeping it as an option is a reasonable compromise

fun `create column with infer type`() {
val data: List<Any> = listOf(1, 2, 3)
val res = data.toDataFrame {
"e" from inferType { it }
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here, it's good it exists, but I think it should be done by default when using expr

@Jolanrensen
Copy link
Collaborator

I think this requires more explanation, as expr already has the ability to generate a type. However, this type is only known if it is supplied to expr as reified parameter. If a column needs to be added using a normal generic function, this type is not known and the newly added column will always be Any/Any?.
This PR adds the option to infer the (schema) type from the data even if the reified type is too broad.

@koperagen
Copy link
Collaborator Author

This summarizes the issue how i initially found it. There was no way to create anything other than Any column from my list of Any that i got from a library. Changes in AddDsl were made for consistency of expr functions

 val data: List<Any> = listOf(1, 2, 3)
val res = data.toDataFrame {
      "d" from { it } // Any :( 
      "e" from inferType { it } // Int
       expr(infer = Infer.Type) { it } into "d" // Int
}

@Jolanrensen Jolanrensen self-requested a review February 13, 2024 13:57
@koperagen koperagen merged commit 739133b into master Feb 13, 2024
1 of 2 checks passed
@koperagen koperagen self-assigned this Feb 21, 2024
@koperagen koperagen added this to the 0.13.0 milestone Feb 21, 2024
@koperagen koperagen added the enhancement New feature or request label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants