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.

Sign up for GitHub

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

Jump to bottom

Introduce a Value_Type.Null. #6281

Open
4 tasks
radeusgd opened this issue Apr 14, 2023 · 10 comments · May be fixed by #11737
Open
4 tasks

Introduce a Value_Type.Null. #6281

radeusgd opened this issue Apr 14, 2023 · 10 comments · May be fixed by #11737
Assignees
@radeusgd
Labels
-libs Libraries: New libraries to be implemented l-table-column-datatypes p-low Low priority

Comments

@radeusgd
Copy link
Member

radeusgd commented Apr 14, 2023 •
edited
Loading

This is orthogonal to #5872.

Initially, I thought we can live without Value_Type.Null. Each of columns coming from the DB has some proper type.

However, there are cases where this becomes problematic.

When I do Table.new [["X", [Nothing]]] - what type X should have? I guess for in-memory it's ok if it's mixed by default and we can cast it to any nullable type we want, and all will work OK.

However, there is the case of creating 'pure NULL' column in Expressions, both in-memory and in-DB.
Currently the behaviour is as follows:

t = table_builder [["X", [1, 2, 3]]]
t.compute "not(Nothing)" . should_fail_with Invalid_Value_Type
t.compute "Nothing + Nothing" . should_fail_with Illegal_Argument
t.compute "Nothing * Nothing" . should_fail_with Invalid_Value_Type

So any expression on nulls fails with Invalid Value Type, that's because the value is Mixed (in-memory) or Unsupported_Data_Type "NULL" (in-DB, at least in Postgres, I'm not even checking SQLite 🤕).

I'm not sure if that behaviour is right. It feels that if t.at "X" + Nothing works OK (if X is itself a numeric column), probably [X] + Nothing but also Nothing + [X] should work too.

The sane way to make it work that comes to my mind is only by adding a Value_Type.Null which these all-null columns can take. Such a special type can then be accepted by any Value_Type.expect_* operation, making these operations work.

  • Add the Null type. It should not be mapped to-from SQL by default, I think.
  • Make sure all expect_* functions also accept the Null type.
    • Should is_* answer True to it too? Figure out, initial thought is that probably not.
  • Add tests for handling of these NULL columns in expressions.
  • Update tests in Aggregate_Column_Spec.enso.
@radeusgd radeusgd added -libs Libraries: New libraries to be implemented l-table-column-datatypes labels Apr 14, 2023
@radeusgd
Copy link
Member Author

I'm not absolutely certain if we need this. If we want to keep the current behavior and accept the errors on Null in these cases, we can close this issue. But I think ideally, we should at some point make sure that expressions and raw column operations are more consistent. So I'd be for implementing this, but probably with a lower priority.

cc: @jdunkerley what do you think about this? I'd love to hear your opinion.

@radeusgd radeusgd added the p-lowest Should be completed at some point label Apr 14, 2023
@jdunkerley
Copy link
Member

Yes we should have a Value_Type.Null it is useful as can be "coerced" to whatever is needed.
Expressions needed this.

@jdunkerley jdunkerley added this to the Design Partners milestone Apr 18, 2023
@jdunkerley jdunkerley moved this from ❓New to 📤 Backlog in Issues Board Apr 18, 2023
@jdunkerley jdunkerley removed this from the Beta Release milestone Jul 13, 2023
@jdunkerley jdunkerley added p-low Low priority and removed p-lowest Should be completed at some point labels Sep 5, 2023
@radeusgd
Copy link
Member Author

  • We should also update tests that were disabled until this is fixed.
  • In particular, we want to update the Union type unification logic so that Null columns are ignored when computing the resulting type (e.g. Null+Integer->Null, not Mixed), only if all columns that are being merged are Null then the result is Null.

radeusgd added a commit that referenced this issue Nov 13, 2024
@radeusgd
disable tests, to be re-enabled with #6281
ef4269b
radeusgd added a commit that referenced this issue Nov 14, 2024
@radeusgd
disable tests, to be re-enabled with #6281
889c84b
radeusgd added a commit that referenced this issue Nov 14, 2024
@radeusgd
yet another edge case for #6281
68df118
@enso-bot enso-bot bot mentioned this issue Nov 15, 2024
Closed
radeusgd added a commit that referenced this issue Nov 15, 2024
@radeusgd
disable tests, to be re-enabled with #6281
4d26650
radeusgd added a commit that referenced this issue Nov 15, 2024
@radeusgd
yet another edge case for #6281
8352acb
@enso-bot
Copy link

enso-bot bot commented Nov 20, 2024

Radosław Waśko reports a new STANDUP for yesterday (2024-11-19):

Progress: Checking a copy_to bug. Created a PR updating to Cloud API changes. Started work on Value_Type.Null - added the new type and updating related functions. WIP. It should be finished by 2024-11-27.

Next Day: Next day I will be working on the same task. Meetings, continue Value_Type.Null.

@enso-bot
Copy link

enso-bot bot commented Nov 21, 2024

Radosław Waśko reports a new STANDUP for yesterday (2024-11-20):

Progress: London meetup. Put up a PR removing stack traces for expected exceptions in the HTTP tests that were confusing in CI logs. It should be finished by 2024-11-27.

Next Day: Next day I will be working on the same task. Meetings, continue Value_Type.Null.

@enso-bot
Copy link

enso-bot bot commented Nov 22, 2024

Radosław Waśko reports a new STANDUP for yesterday (2024-11-21):

Progress: London meetup. It should be finished by 2024-11-27.

Next Day: Next day I will be working on the same task. Meetings, continue Value_Type.Null.

@enso-bot
Copy link

enso-bot bot commented Nov 25, 2024

Radosław Waśko reports a new STANDUP for the provided date (2024-11-22):

Progress: London meetup. It should be finished by 2024-11-27.

Next Day: Next day I will be working on the same task. Continue Value_Type.Null.

This was referenced Nov 27, 2024
Merged
Open
@enso-bot
Copy link

enso-bot bot commented Dec 3, 2024

Radosław Waśko reports a new STANDUP for yesterday (2024-12-02):

Progress: Catching up advent of code. Finishing implementation of Value_Type.Null. Almost all tests passing. Adding more edge cases for read many, discussing and implementing them. Investigating fix for Data.list failure on Enso Cloud files, added unit tests. It should be finished by 2024-12-03.

Next Day: Next day I will be working on the same task. Finish Value_Type.Null, start next tasks.

@radeusgd radeusgd moved this from 🔧 Implementation to 👁️ Code review in Issues Board Dec 3, 2024
@enso-bot
Copy link

enso-bot bot commented Dec 4, 2024

Radosław Waśko reports a new STANDUP for yesterday (2024-12-03):

Progress: Fixing remaining test failures in Null Value Type. Prepared a PR. It should be finished by 2024-12-04.

Next Day: Next day I will be working on the #11747 task. Fix remaining tests. Start work on Data.list through datalinks.

@enso-bot
Copy link

enso-bot bot commented Dec 5, 2024

Radosław Waśko reports a new STANDUP for yesterday (2024-12-04):

Progress: Fixing remaining tests, started work on Data.list through datalinks - clarifying spec, tests. It should be finished by 2024-12-04.

Next Day: Next day I will be working on the #11747 task. Continue work on Data.list / symlinks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@radeusgd radeusgd

Labels
-libs Libraries: New libraries to be implemented l-table-column-datatypes p-low Low priority
Projects
Issues Board
Status: 👁️ Code review
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Introduce Value_Type.Null, fix edge cases for read_many enso-org/enso
2 participants
@radeusgd @jdunkerley