Implement `cast` for Table and Column #6711

radeusgd · 2023-05-16T13:17:40Z

Pull Request Description

Closes #6112

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

distribution/lib/Standard/Table/0.0.0-dev/src/Internal/Cast_Helpers.enso

jdunkerley

First few comments will review once done morning meetings.

distribution/lib/Standard/Database/0.0.0-dev/src/Data/Column.enso

jdunkerley · 2023-05-18T08:57:56Z

distribution/lib/Standard/Database/0.0.0-dev/src/Data/Column.enso

-                SQL_Type_Reference.new self.connection self.context new_expression
-            new_column = dialect.make_cast self.as_internal target_sql_type infer_from_database
-            Column.Value new_column.name self.connection new_column.sql_type_reference new_column.expression self.context
+    cast self value_type=Value_Type.Char on_problems=Problem_Behavior.Report_Warning =


Lets remove the value_type default and make the user choose.

Hmm, but why?

We wanted to have as many defaults on nodes as possible, right? So that they preview nicely in the CB when 'pre-selected'.

While for parse we should remove the default in Database, because there is no good choice; I think for cast Char is a pretty good choice - converting to Text should work in baiscally every case and is a good example of what cast does.

I can ofc change it if you think this is the right approach, but IMO this particular default is OK, so I'd like to confirm that you really want this and would appreciate understanding why.

I feel like in cast it is a real choice of the user so I felt like it should be a users explicit choice.

I see. But if we go by this heuristic, shouldn't we also remove defaults from stuff like select_columns, remove_columns and many more? There it is even more of an explicit user's choice which columns they want to remove - defaulting to the first one is just an arbitrary default we have to show the functionality working in the preview.

(I don't want to be stubborn but I want us to be consistent and really understand the reasoning for why we want to include defaults in X but not in Y.)

I agree - and ok lets go with Text as the default I guess as its harmless.

I'm not sure we should have defaults on everything and this feels a more aggressive default.

I'm not sure we should have defaults on everything

Yeah, I think I agree.

and this feels a more aggressive default.

But I cannot understand this part - hence my opposition.

It's not super important, and I guess a bit subjective so it may be hard to explain - but could you try to narrow down what makes this a 'more aggresive' default?

I think my dissonance comes from the fact that IMO select_columns and remove_columns defaults are even 'more aggresive' than this one. Or maybe you agree that these are 'aggressive defaults' too?

(I know it's not the most important discussion, but just trying to understand your perspective so I can understand the decisions better)

It feels like it's changing data in a way - in select it's basically a no-op (we just pick a column from the list).
On a billion-row table, cast is a chunky complex operation and hence I guess felt like worth getting user to choose what they want to do. Unlike parse, we can't guess.

Ah, I didn't think of the operation cost! That is a very good point indeed - thanks for explaining it.

So I can indeed remove the default, we indeed don't want to wait for a very long operation to complete just to switch the type...

distribution/lib/Standard/Database/0.0.0-dev/src/Data/Column.enso

distribution/lib/Standard/Database/0.0.0-dev/src/Data/Table.enso

jdunkerley

Looks good but have put some questions in.

distribution/lib/Standard/Database/0.0.0-dev/src/Internal/SQLite/SQLite_Dialect.enso

std-bits/table/src/main/java/org/enso/table/data/column/storage/BoolStorage.java

std-bits/table/src/main/java/org/enso/table/data/column/storage/DateStorage.java

std-bits/table/src/main/java/org/enso/table/data/column/storage/DateTimeStorage.java

std-bits/table/src/main/java/org/enso/table/data/column/storage/DoubleStorage.java

std-bits/table/src/main/java/org/enso/table/data/column/storage/LongStorage.java

std-bits/table/src/main/java/org/enso/table/data/column/storage/StringStorage.java

JaroslavTulach · 2023-05-19T07:30:01Z

std-bits/table/src/main/java/org/enso/table/data/column/builder/object/Builder.java

-      case TimeOfDayType() -> new TimeOfDayBuilder(size);
-      case FloatType(Bits bits) ->
-        switch (bits) {
+      case AnyObjectType x -> new ObjectBuilder(size);


Yup, this will prevent the incremental compilation problems as matching on type doesn't need to recognize record.

Yeah, I was sad to see the neat matching go, but better build stability is more important.

Akirathan

Nice, so cool that we will finally get rid of the incremental compilation issue with records

Implements the simplest `parse` scenarios for the Database backend. Before #6711 these could have been done by `cast`, but in #6711 the APIs were unified to only allow casting to the same set of types in both in-memory and Database. Converting Text to other types is supposed to be done by `parse` and not `cast`, so the ability to use `cast` for rudimentary parsing is removed in the Database backend to make it consistent with in-memory. But now it is lacking any, even simplest, Text->Int/Text->Date support. To alleviate that, the simple scenarios for `parse` are implemented (no support for format customization yet, will boil down to a cast under the hood).

…t-rename * develop: Widgets, Vector as Column, Viz Fixes and Rename Columns (#6768) Implement simple variants of `parse` for the Database backend (#6731) Enable `require-jsdoc` lint and add two lints related to React (#6403) Decimal/Integer .round and .int #6654 (#6743) Set suggestion reexports when serializing the library (#6778) Fix file uploading node expression. (#6689) Using WarningsLibrary to query for warnings (#6751) Implement `cast` for Table and Column (#6711) Display Initializing project... message when initializing project (#6661) Only send suggestions updates when type changes (#6755) sbt runEngineDistribution --debug to ease debuggging (#6745) Display "modified at" column on the cloud dashboard (#6687) Meta.meta Integer . methods (#6740) Show spinner while loading directory (#6714) Add cloud dashboard to changelog (#6688) Fix list editor panics during insertion (#6540) Update bug-report.yml Remove project create form (#6710) Change full-screen visualisation shortcut to shift-space (#6663)

* develop: (30 commits) Widgets, Vector as Column, Viz Fixes and Rename Columns (#6768) Implement simple variants of `parse` for the Database backend (#6731) Enable `require-jsdoc` lint and add two lints related to React (#6403) Decimal/Integer .round and .int #6654 (#6743) Set suggestion reexports when serializing the library (#6778) Fix file uploading node expression. (#6689) Using WarningsLibrary to query for warnings (#6751) Implement `cast` for Table and Column (#6711) Display Initializing project... message when initializing project (#6661) Only send suggestions updates when type changes (#6755) sbt runEngineDistribution --debug to ease debuggging (#6745) Display "modified at" column on the cloud dashboard (#6687) Meta.meta Integer . methods (#6740) Show spinner while loading directory (#6714) Add cloud dashboard to changelog (#6688) Fix list editor panics during insertion (#6540) Update bug-report.yml Remove project create form (#6710) Change full-screen visualisation shortcut to shift-space (#6663) Revert "Show spinner when opening/creating a project (#6321)" (#6712) ...

Follow-up of #6711 Closes #6838

radeusgd self-assigned this May 16, 2023

radeusgd force-pushed the wip/radeusgd/6112-table-cast branch 2 times, most recently from d3bc5d9 to 9b6b641 Compare May 17, 2023 11:54

radeusgd marked this pull request as ready for review May 17, 2023 11:56

radeusgd requested review from jdunkerley and GregoryTravis as code owners May 17, 2023 11:56

radeusgd mentioned this pull request May 17, 2023

Implement simple variants of parse for the Database backend #6731

Merged

5 tasks

GregoryTravis approved these changes May 17, 2023

View reviewed changes

distribution/lib/Standard/Table/0.0.0-dev/src/Internal/Cast_Helpers.enso Show resolved Hide resolved

radeusgd force-pushed the wip/radeusgd/6112-table-cast branch from dff87c3 to 80c22ef Compare May 17, 2023 16:54

jdunkerley reviewed May 18, 2023

View reviewed changes

radeusgd force-pushed the wip/radeusgd/6112-table-cast branch from 80c22ef to 110071b Compare May 18, 2023 09:55

radeusgd requested a review from jdunkerley May 18, 2023 09:56

jdunkerley approved these changes May 18, 2023

View reviewed changes

radeusgd force-pushed the wip/radeusgd/6112-table-cast branch from 110071b to 2a12295 Compare May 18, 2023 17:34

radeusgd requested review from 4e6, JaroslavTulach, hubertp and Akirathan as code owners May 18, 2023 17:34

radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label May 18, 2023

radeusgd added 11 commits May 18, 2023 19:47

Adding tests for cast/parse

881b972

checkpoint

f4e4d23

change default arg in Column.cast

0ccc6d6

checkpoint

43c5ad6

checkpoint

12fb2b5

implemented storage conversions

c869360

fix tests for Postgres

7e2704b

fixes

6337cc5

check overflows in float->int, currently no warning yet

2ba1100

fixing tests

8092768

Update gitignore

e53cd9f

radeusgd added 13 commits May 18, 2023 19:47

update Lossy_Conversion

0f332a8

implement problem reporting

0c88f32

javafmt

1cb60ce

changelog

ecdf4fd

cr1

68ef6fd

cr2

5b528af

fix!

d83d0c8

error on missing columns

6914765

cr3: remove the default

9809440

cr4

fa1d865

formatting

34fdd74

rename Lossy_Conversion to Conversion_Failure

df400f4

fixes

32f9772

radeusgd force-pushed the wip/radeusgd/6112-table-cast branch from 2a12295 to 32f9772 Compare May 18, 2023 17:47

JaroslavTulach reviewed May 19, 2023

View reviewed changes

Akirathan approved these changes May 19, 2023

View reviewed changes

JaroslavTulach approved these changes May 19, 2023

View reviewed changes

mergify bot merged commit 447786a into develop May 19, 2023

mergify bot deleted the wip/radeusgd/6112-table-cast branch May 19, 2023 10:00

radeusgd mentioned this pull request May 19, 2023

Allow casting a Mixed column into a concrete type #6777

Merged

5 tasks

radeusgd mentioned this pull request May 24, 2023

Allow casting a Mixed type column into a specific type, selecting elements from the column that match the target type #6838

Closed

mergify bot pushed a commit that referenced this pull request May 26, 2023

Allow casting a Mixed column into a concrete type (#6777)

c3e771c

Follow-up of #6711 Closes #6838

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `cast` for Table and Column #6711

Implement `cast` for Table and Column #6711

radeusgd commented May 16, 2023 •

edited

Loading

jdunkerley left a comment

jdunkerley May 18, 2023

radeusgd May 18, 2023

jdunkerley May 18, 2023

radeusgd May 18, 2023

jdunkerley May 18, 2023

radeusgd May 18, 2023

radeusgd May 18, 2023

jdunkerley May 18, 2023

radeusgd May 18, 2023

jdunkerley left a comment

JaroslavTulach May 19, 2023

radeusgd May 19, 2023

Akirathan left a comment

Implement cast for Table and Column #6711

Implement cast for Table and Column #6711

Conversation

radeusgd commented May 16, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

jdunkerley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdunkerley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akirathan left a comment

Choose a reason for hiding this comment

Implement `cast` for Table and Column #6711

Implement `cast` for Table and Column #6711

radeusgd commented May 16, 2023 •

edited

Loading