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 value_type support to from_vector, map and zip in in-memory Column. #6111

Closed
4 tasks done
radeusgd opened this issue Mar 28, 2023 · 4 comments · Fixed by #7637
Closed
4 tasks done

Add value_type support to from_vector, map and zip in in-memory Column. #6111

radeusgd opened this issue Mar 28, 2023 · 4 comments · Fixed by #7637
Assignees
Labels
-libs Libraries: New libraries to be implemented l-table-column-datatypes

Comments

@radeusgd
Copy link
Member

radeusgd commented Mar 28, 2023

With the value types implemented, we need to update these three functions.

All three of them should take an optional value_type parameter. It will default to Auto.

  • In the Auto mode, the current behaviour will be retained - the InferredBuilder will be used under the hood, figuring out the type that fits the provided data.
  • If a specific value type is provided, a builder for the requested type will be used instead.
    • Similar but stricter rules like with cast should be used:
      • if a value does not match the requested type (text-integer mismatch) or fit its size (text-length restriction / integer bit size), always a type mismatch is raised.
  • Add tests for all 3 methods handling the Auto case, the specific types and checking for lossy conversion reports.
@radeusgd radeusgd self-assigned this Mar 28, 2023
@radeusgd radeusgd added -libs Libraries: New libraries to be implemented l-table-column-datatypes labels Mar 28, 2023
@radeusgd
Copy link
Member Author

@jdunkerley what do you think, if we get a type mismatch, do we throw an Invalid_Value_Type error, or report it as a warning and replace the value with Nothing?

I would throw an error in this case, as one can always use Auto and then do the cast. So if they are using the specific return type override, they probably want more correctness assurance. But I'm not strongly attached to this idea, so I'd appreciate your opinion on this.

@jdunkerley
Copy link
Member

yes lets throw and error and make the user choose explicitly

@enso-bot
Copy link

enso-bot bot commented Aug 22, 2023

Radosław Waśko reports a new STANDUP for today (2023-08-22):

Progress: Catching up on bookclub. Reporting some bugs found. More reviews. Meetings. Trying out a thing to clean up a test suite in Standard.Test and instead move it back to Tests and do cross-project imports - experiment turned out to be more complicated and became a standalone issue for later: #7633. Started work on value_type in Column.from_vector. Imlpemented the happy path and tests for from_vector - both happy and error handling tests. Adding tests for Column.map. It should be finished by 2023-08-24.

Next Day: Next day I will be working on the same task. Add other tests for map and zip. Implement the required error handling.

@enso-bot
Copy link

enso-bot bot commented Aug 23, 2023

Radosław Waśko reports a new STANDUP for today (2023-08-23):

Progress: Added remaining tests. Implemented error handling. Enabled some old tests that should work now and adapted them - fixing some minor issues along the way. PR is basically ready for review. It should be finished by 2023-08-24.

Next Day: Next day I will be working on the #7633 task. Fix one not directly related issue. Look into next task.

@mergify mergify bot closed this as completed in #7637 Aug 31, 2023
mergify bot pushed a commit that referenced this issue Aug 31, 2023
… `Column.map` and `Column.zip` (#7637)

- Closes #6111
- Aligns semantics of handling Mixed columns.
- Now, if an operation like `iif` or `fill_nothing` is given a `Mixed` column, the result will also be `Mixed` regardless of the `inferred_precise_value_type`.
- Enables a few old tests that were pending but could be enabled since the types work is advanced enough.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-libs Libraries: New libraries to be implemented l-table-column-datatypes
Projects
Archived in project
2 participants