Support for `Table` output in `Data.read_many` #11546

radeusgd · 2024-11-13T13:21:21Z

Pull Request Description

Closes Implement Data.read_many #11311

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.

radeusgd · 2024-11-14T14:01:03Z

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

+type Return_As_Table
+    ## Returns a table with a new column `Value` containing the objects loaded
+       from each file.
+
+       When the source for files to load was a table, all columns from the
+       original table are also retained. In case of name clashes, the newly
+       added columns will get a suffix.
+
+       When the source was a simple Vector, the returned table will also contain
+       a `File Name` column.
+    Table_Of_Objects


The name Table_Of_Objects was a working name. I'm not sure if we want to keep it or come up with something better?

Definitely Table_Of_Tables in Excel sounded much better. But here it may not necessarily be tables, it may be various objects. So Table_Of_Objects is quite precise, but it sounds weird.

Maybe we should change approach altogether and try something like Added_Column? (meaning that the loaded files are added as a column to the input)

Values_Column

Table_With_New_Column

any thoughts on these names?

I think Table_With_New_Column or Table_Of_Objects seem the best to me, Values_Column I think could end up being confusing

How about more of a boolean? Merged/UnMerged. Merged True/False? I feel like it would be hard for someone to guess what table_of_objects is going to do without reading the help or trying it

I wonder if:

type Return_As ## Single table with values expanded Merged_Table ## PRIVATE Add this for backwards compatiblity then hide in widget? Table_Of_Tables ## Individual_Cells ## Objects returned as a raw vector allowing zipping to input Vector

Not sure on name yet either - will think some more

Yes sorry that wasn't clear. When I said boolean I didn't mean a boolean type. More text options that were Merged or Not Merged.

Vector is handled by Return_As_Base - the type needs to be split up so that the Vector return works even if Table is not imported. It is opaque to the user - the dropdown displays all available types and autoscoping works as shown in tests.

I'd not add Table_Of_Tables to Data.read_many. It is supported in Excel_Workbook.read_many to retain backwards compatibility. But adding it to a brand new function seems unnecessary - why add an obsolete option?

I was in my head thinking of it as a single type used by both functions but I appreciate we are actually building this dynamically. Just for working out how it might look in the dropdown thinking as a single type is a bit easier!

If the ..Table_Of_Tables was an option not shown in the dropdown but in one of the mix in types then no special handling would be needed for the two reads to share the same object was my thought.

I wonder if:

..As_Vector

..With_New_Column

..As_Merged_Table

Might be cleanest?

If the ..Table_Of_Tables was an option not shown in the dropdown but in one of the mix in types then no special handling would be needed for the two reads to share the same object was my thought.

But then more code would be affected by the legacy options, I'd rather not grow places that take legacy options when not needed. Unless there's any problem with the current approach, I'd keep the compatibility conversion as-is.

I wonder if:

..As_Vector

..With_New_Column

..As_Merged_Table

Might be cleanest?

That sounds quite clear to me 👍

Yes I like those options

The options outlined in the comment above have been implemented. Excel_Workbook.read_many also allows old options for compatibility, but Data.read_many (the new component) only works with new ones.

radeusgd · 2024-11-14T14:14:45Z

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

@@ -16,146 +7,3 @@ type Match_Columns

       Note: column names are not compared.
    By_Position
-


No meaningful code changes in this file, just moving the helpers to separate module - Match_Columns_Helpers.

radeusgd · 2024-11-27T16:02:38Z

Updating the widget to be a vector of file pickers.
Fixing In_List to throw missing argument.

radeusgd · 2024-11-27T16:09:11Z

Maybe out of scope for this MR, but this warning is very long

Warning: No common type was found for types: Char (variable length, max_size=unlimited), Char (variable length, max_size=unlimited), Char (variable length, max_size=unlimited), Integer (64 bits) when unifying column [Sales], so the values were converted to text. If you want to have mixed types instead, please cast one of the columns to Mixed beforehand.

Wonder if we should

Remove the duplicate types

Remove the last sentence about Mixed

So then

Warning: No common type was found for types: Char (variable length, max_size=unlimited), Integer (64 bits) when unifying column [Sales], so the values were converted to text.

Sure that's an easy change - just added distinct and removed the suffix, now we have:

…API for now TODO: to be revisited in #11681

radeusgd · 2024-11-27T18:27:46Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Read/Return_As.enso

+                # TODO ideally we'd like to use `Meta` but it currently has no support for UnresolvedConstructor so we use a 'hack'.
+                is_the_default = value.to_text == "..As_Merged_Table"


Checking to_text feels like a bit of a hack, but currently Meta seems to lack support for UnresolvedConstructor - Meta.meta returns Primitive and does not allow any inspecting.

It feels like it may prove useful in the future, @jdunkerley do we want to create a ticket for the engine team to add support for Unresolved_Constructor in Meta?

I would have done it via

type My_Private_Type As_Merged_Table

and trying to convert to that.

I agree we could probably do with something on Meta.
cc: @JaroslavTulach for visibility.

jdunkerley · 2024-11-27T20:38:51Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Read/Return_As.enso

+                # TODO ideally we'd like to use `Meta` but it currently has no support for UnresolvedConstructor so we use a 'hack'.
+                is_the_default = value.to_text == "..As_Merged_Table"


I would have done it via

type My_Private_Type As_Merged_Table

and trying to convert to that.

I agree we could probably do with something on Meta.
cc: @JaroslavTulach for visibility.

… is undesirable

…in test

- Closes #11311 (cherry picked from commit dc55b4e)

radeusgd added the CI: No changelog needed Do not require a changelog entry for this PR. label Nov 13, 2024

radeusgd self-assigned this Nov 13, 2024

radeusgd force-pushed the wip/radeusgd/11311-read-many-to-table branch 2 times, most recently from 4c4b4e0 to 68df118 Compare November 14, 2024 13:44

radeusgd marked this pull request as ready for review November 14, 2024 13:58

radeusgd requested review from jdunkerley, GregoryTravis, AdRiley and marthasharkey as code owners November 14, 2024 13:58

radeusgd commented Nov 14, 2024

View reviewed changes

radeusgd added 19 commits November 15, 2024 14:23

WIP merging tables new type

8326daf

basic logic for returning tables and merging

e50e0b0

WIP dependent defaults

ff3d6ab

updating tests

73ee0e0

big merge test

a7026cd

more tests

bb202ce

fixes to make it compile

faf4c8e

checkpoint

02fa764

adding/fixing widget

813f8cb

fix test

8af415a

better to_text for Excel Workbook

7cd6f71

move excel tests, but keep for later

4825597

checkpoint widgets, adding args, moving towards union

806733a

WIP

6281296

WIP

9d1751d

compat

da7b165

ensure backwards compatibility for Excel read_many

6fe5f33

adding tests for union behaviour, empty tables, mixing types etc.

2c59261

spec for Excel

73f906f

GregoryTravis approved these changes Nov 27, 2024

View reviewed changes

radeusgd added 2 commits November 27, 2024 17:04

Merge branch 'develop' into wip/radeusgd/11311-read-many-to-table

1ada3bc

test that read_many works with cloud files too

98c0e4c

radeusgd added 6 commits November 27, 2024 17:09

CR: shorter warning text

ebd2cbf

CR: adding more warning tests to catch a problem

c4ba842

fix spurious warning

fccaff5

CR: hide customizing handling of Dictionary in expansion from public …

6cc0e8a

…API for now TODO: to be revisited in #11681

CR: change the default to always be As_Merged_Table

2d6fa4a

special handling for default

00f572a

radeusgd requested review from AdRiley and jdunkerley November 27, 2024 18:26

radeusgd commented Nov 27, 2024

View reviewed changes

jdunkerley approved these changes Nov 27, 2024

View reviewed changes

radeusgd added 6 commits November 28, 2024 09:29

Merge branch 'develop' into wip/radeusgd/11311-read-many-to-table

456fff3

CR: nicer check of autoscoped constructor

694a1aa

move widget test - it was causing Base_Tests to depend on Table which…

9c09323

… is undesirable

fix - default return type is no longer Vector, so argument is needed …

95ad79b

…in test

fix test - there are also duplicate name warnings so only was wrong

495e0bb

Merge branch 'develop' into wip/radeusgd/11311-read-many-to-table

fc9e26b

radeusgd added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Nov 28, 2024

Merge branch 'develop' into wip/radeusgd/11311-read-many-to-table

fa69bdf

radeusgd mentioned this pull request Nov 29, 2024

Native Image run includes SPI for libraries that are not loaded, leading to crashes #11707

Open

radeusgd added 2 commits November 29, 2024 11:35

workaround for #11707

84c0692

Merge branch 'develop' into wip/radeusgd/11311-read-many-to-table

a600180

radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Nov 29, 2024

mergify bot merged commit dc55b4e into develop Nov 29, 2024
39 checks passed

mergify bot deleted the wip/radeusgd/11311-read-many-to-table branch November 29, 2024 12:06

jdunkerley pushed a commit that referenced this pull request Nov 29, 2024

Support for Table output in Data.read_many (#11546)

63148f3

- Closes #11311 (cherry picked from commit dc55b4e)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for `Table` output in `Data.read_many` #11546

Support for `Table` output in `Data.read_many` #11546

radeusgd commented Nov 13, 2024 •

edited

Loading

radeusgd Nov 14, 2024

radeusgd Nov 14, 2024

marthasharkey Nov 14, 2024

AdRiley Nov 14, 2024

jdunkerley Nov 14, 2024 •

edited

Loading

AdRiley Nov 15, 2024

jdunkerley Nov 15, 2024

radeusgd Nov 15, 2024

AdRiley Nov 15, 2024

radeusgd Nov 26, 2024

radeusgd Nov 14, 2024

radeusgd commented Nov 27, 2024

radeusgd commented Nov 27, 2024

radeusgd Nov 27, 2024

jdunkerley Nov 27, 2024

jdunkerley Nov 27, 2024

		@@ -16,146 +7,3 @@ type Match_Columns

		Note: column names are not compared.
		By_Position

		# TODO ideally we'd like to use `Meta` but it currently has no support for UnresolvedConstructor so we use a 'hack'.
		is_the_default = value.to_text == "..As_Merged_Table"

Support for Table output in Data.read_many #11546

Support for Table output in Data.read_many #11546

Conversation

radeusgd commented Nov 13, 2024 • 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

jdunkerley Nov 14, 2024 • edited Loading

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

radeusgd commented Nov 27, 2024

radeusgd commented Nov 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Support for `Table` output in `Data.read_many` #11546

Support for `Table` output in `Data.read_many` #11546

radeusgd commented Nov 13, 2024 •

edited

Loading

jdunkerley Nov 14, 2024 •

edited

Loading