Refine single-column Table to act as a Column #12165

radeusgd · 2025-01-28T12:23:22Z

Pull Request Description

Closes Allow a Table that has just 1 column to act as Column #12137

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,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.
If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
or the Snowflake database integration, a run of the Extra Tests has been scheduled.
- If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

… point'

…sure the copy-pasted code is not buggy...

…ited` method

…ited`

radeusgd · 2025-01-29T18:00:02Z

distribution/lib/Standard/Database/0.0.0-dev/src/Connection/Connection.enso

-    result.catch SQL_Error _->
+    if result.is_error then
        Error.throw (Table_Not_Found.Error name)
+    result


Since #11777 we can simplify the error handling code, let's use it :)

radeusgd · 2025-01-29T18:01:09Z

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

+    ## Converts all table contents to a text value in delimited format.
+
+       Arguments:
+       - format: Allows to customize the delimiter and other settings of the
+         format. Defaults to tab-separated values.
+    to_delimited self (format:Delimited_Format = ..Delimited '\t') -> Text =
+        Delimited_Writer.write_text self format


We have agreed that Text.from Table conversion wasn't that useful and so we are turning it into a method on Table instead.

Does that name seem good or any suggestions for a better one?

Matches with to_xml

jdunkerley · 2025-01-29T18:16:35Z

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

+    ## Converts all table contents to a text value in delimited format.
+
+       Arguments:
+       - format: Allows to customize the delimiter and other settings of the
+         format. Defaults to tab-separated values.
+    to_delimited self (format:Delimited_Format = ..Delimited '\t') -> Text =
+        Delimited_Writer.write_text self format


Matches with to_xml

jdunkerley · 2025-01-29T18:18:23Z

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

+refine_table (table : Table) =
+    if is_single_column table . not then table else
+        column = table.at 0
+        # This should be consistent with `Column_Refinements.refine_column` - the code needs to be copied because we need to spell out all the types.


Probably one we need to talk over with Jaroslav to see if we can do anything better longer term.

GregoryTravis · 2025-01-29T21:07:10Z

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

+            Value_Type.Time -> (table : Table & Column & Time_Of_Day)
+            Value_Type.Date_Time True -> (table : Table & Column & Date_Time)
+            Value_Type.Decimal _ scale ->
+                is_integer = scale == 0


What about scale < 0? It can still be an integer, so would Table & Column & Integer be the right type in that case?

Good point! I forgot it can be negative.

Updated in bc718a2

GregoryTravis · 2025-01-29T21:10:34Z

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

@@ -77,26 +77,26 @@ _merge_input_and_tables (input_table : Table) (tables_for_rows : Vector Read_Man

        multiplicated_inputs = duplicate_rows input_table counts
        Runtime.assert (unified_data.row_count == multiplicated_inputs.row_count)
-        Runtime.assert (unified_metadata.is_nothing || (unified_metadata.row_count == unified_data.row_count))
+        Runtime.assert ((Nothing == unified_metadata) || (unified_metadata.row_count == unified_data.row_count))


Why is this change needed?

Due to #12185.

Essentially - unified_metadata is a Table. But if it contains 1 column, it also now is a & Column. And due to #12185 bug, when calling unified_metadata.is_nothing, instead of dispatching to the Any.is_nothing implementation, it dispatches to Column.is_nothing and returns Column instead of Boolean - the assert then crashes as it expects a boolean.

Nothing == x goes around this issue and checks the is this a Nothing value property correctly.

GregoryTravis · 2025-01-29T21:22:39Z

test/Table_Tests/src/Database/Common/Single_Column_Table_Spec.enso

+            t1 = setup.table_builder [["A", [23]]]
+            t1.should_be_a DB_Table
+            (t1:DB_Column).name.should_equal "A"
+            Test.expect_panic Type_Error (t1:Integer).to_vector


I am confused by this test. to_vector is not a method of Integer, so I would expect a method-not-found error here.

Right, it is a typo, thanks for noticing!

The test was still succeeding because it t1:Integer is (expectedly) failing with Type_Error. So it never reached the to_vector method call... But it shouldn't have been there, I removed it.

AdRiley · 2025-01-30T09:26:03Z

distribution/lib/Standard/Database/0.0.0-dev/src/DB_Table.enso

+    ## The internal constructor used to construct a DB_Table instance.
+
+       It can perform some additional operations, like refining the type,
+       so it should always be preferred over calling `DB_Table.Value` directly.


How would I know this without reading this comment? Guess we need some sort of convention for module private constructors similar to what we have for module priavte methods

Good point, I will add a comment in Value constructor that it should not be used directly.

We can also consider changing the name of the constructor from Value to something like Internal_Value or Internals - to make it more apparent that this is some internal thing that should only be used in limited places. What do you think?

AdRiley

Seeing it in use has led me to a new question. (And I think this is the question that has been in my head all week trying to get out. :))

If the types are hidden then how are they any use? And I think the answer we have been saying is: well they can be cast to the hidden types. And the GUI will cast them to the hidden types. But how will the GUI know it can cast them to the hidden types if they are hidden?

radeusgd · 2025-02-03T12:07:08Z

But how will the GUI know it can cast them to the hidden types if they are hidden?

@AdRiley The idea is that the GUI should be able to 'extract' even the 'hidden' part of the type.

As far as I understand it was supposed to be handled by PR #11583 (and perhaps some followup changes). I think the idea is that the types sent to the IDE include not only the 'visible' but also the 'hidden' part. Now - I think the message currently does not distinguish which part is which - it just sends all the types a value has. Perhaps we will need to expand the protocol to include such a distinction (e.g. if we only wanted to insert the casts for hidden types, as including it for visible ones may be redundant).

# Conflicts: # test/Base_Tests/src/Semantic/Multi_Value_As_Type_Refinement_Spec.enso

…t because original type should be preferred Note: a Column may be _converted_ to Table but is not a Column&Table, and conversions are not checked in case-of, so the other direction is not a problem

radeusgd self-assigned this Jan 28, 2025

radeusgd added the CI: No changelog needed Do not require a changelog entry for this PR. label Jan 28, 2025

radeusgd added 14 commits January 28, 2025 16:24

use from_java_table instead of Table.Value to get a single 'entry…

d16a695

… point'

rename

3d84772

add single column refinement

9936506

workaround for IR cache bug

0ea4db9

add tests for in-memory

edf7b98

one more test case

28b54a9

add analogous DB_Table tests

5f5fada

fixes

b9af812

implement for DB_Table

d4a4617

some alignment/fixes to tests

eedc808

avoid warning in test

649ce8e

add tests for single column + single value

4f64b0e

copy paste to get single-column single-value table working

4f2f8f5

given we aren't re-using column logic, we should test all cases to en…

7be3f68

…sure the copy-pasted code is not buggy...

radeusgd force-pushed the wip/radeusgd/12137-table-as-single-column branch from 03cd724 to 7be3f68 Compare January 28, 2025 16:29

remove Text.from Table conversion and replace with explicit `to_delim…

334afcd

…ited` method

radeusgd mentioned this pull request Jan 29, 2025

Unexpected ClassCastException upon atom field access when intersection types and warnings are combined #12180

Open

radeusgd added 2 commits January 29, 2025 16:22

test

55d5f60

workaround for #12180

6d2158f

radeusgd mentioned this pull request Jan 29, 2025

Method dispatch ordering: should we traverse all multi values before checking the parent types? #12185

Open

radeusgd added 7 commits January 29, 2025 16:37

workaround for #12185 - stop using is_nothing

ad09f59

another workaround for #12180

fb0f1f8

fix precedence in test

3553c1b

add test

6ee6c9c

workaround for .catch bug, but can stay

85e8bc1

replace usages of removed Text.from Table conversion with `to_delim…

545e4e1

…ited`

Merge branch 'develop' into wip/radeusgd/12137-table-as-single-column

3a8eb76

radeusgd marked this pull request as ready for review January 29, 2025 17:59

radeusgd requested review from jdunkerley, GregoryTravis, AdRiley and marthasharkey as code owners January 29, 2025 17:59

radeusgd commented Jan 29, 2025

View reviewed changes

jdunkerley approved these changes Jan 29, 2025

View reviewed changes

GregoryTravis reviewed Jan 29, 2025

View reviewed changes

AdRiley reviewed Jan 30, 2025

View reviewed changes

AdRiley approved these changes Jan 30, 2025

View reviewed changes

radeusgd added 4 commits February 3, 2025 13:48

Merge branch 'develop' into wip/radeusgd/12137-table-as-single-column

e2f311e

# Conflicts: # test/Base_Tests/src/Semantic/Multi_Value_As_Type_Refinement_Spec.enso

CR: single helper for is decimal integer

bc718a2

CR: comments about not using internal constructors

3bf49a2

CR: fix typo in test

c9ee850

radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Feb 3, 2025

radeusgd added 4 commits February 4, 2025 19:12

Merge branch 'develop' into wip/radeusgd/12137-table-as-single-column

18efc0e

name matches as per new conventions

609ae4b

more: name matches as per new conventions

0812321

reorder: a Table may also be a Column, so we need to check Table firs…

fcda638

…t because original type should be preferred Note: a Column may be _converted_ to Table but is not a Column&Table, and conversions are not checked in case-of, so the other direction is not a problem

mergify bot merged commit 7031afe into develop Feb 4, 2025
41 of 44 checks passed

mergify bot deleted the wip/radeusgd/12137-table-as-single-column branch February 4, 2025 22:32

radeusgd mentioned this pull request Feb 6, 2025

Fix tests that falsely assumed table ordering in DB #12248

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refine single-column Table to act as a Column #12165

Refine single-column Table to act as a Column #12165

radeusgd commented Jan 28, 2025 •

edited

Loading

radeusgd Jan 29, 2025

radeusgd Jan 29, 2025

jdunkerley Jan 29, 2025

jdunkerley Jan 29, 2025

jdunkerley Jan 29, 2025

GregoryTravis Jan 29, 2025

radeusgd Feb 3, 2025

radeusgd Feb 3, 2025

GregoryTravis Jan 29, 2025

radeusgd Feb 3, 2025

GregoryTravis Jan 29, 2025

radeusgd Feb 3, 2025

radeusgd Feb 3, 2025

AdRiley Jan 30, 2025

radeusgd Feb 3, 2025

radeusgd Feb 3, 2025

AdRiley left a comment

radeusgd commented Feb 3, 2025

Refine single-column Table to act as a Column #12165

Refine single-column Table to act as a Column #12165

Conversation

radeusgd commented Jan 28, 2025 • edited Loading

Pull Request Description

Important Notes

Checklist

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

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

AdRiley left a comment

Choose a reason for hiding this comment

radeusgd commented Feb 3, 2025

radeusgd commented Jan 28, 2025 •

edited

Loading