Use checked signatures for Table and Column #12283

radeusgd · 2025-02-14T08:15:25Z

Pull Request Description

Closes Update Table/Column operations' signatures to be checked #12138

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.

This reverts commit 3a393f1.

…le that anyway reporting as Invalid_Value_Type

…mments

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

GregoryTravis · 2025-02-17T20:53:52Z

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

@@ -1864,8 +1793,7 @@ type DB_Column
             import Standard.Examples

             example_contains = Examples.text_column_1.is_in [1, 2, 5]
-    is_in : DB_Column | Vector -> DB_Column
-    is_in self vector =
+    is_in self (vector : DB_Column | Vector | Array) -> DB_Column =


Do we consider Array a user-facing type? I rarely see it used anywhere but tests and benchmarks.

Not sure, I guess not necessarily.

But it is here needed to make expressions work (before the type was unchecked so the Array went through). It is the de-facto used type signature.

However, I'm happy to create a ticket to remove | Array from here and fix the related expression syntax issue.

I don't think it's important right now, I was just wondering what the status of Array is.

It is hardly used in public user-facing APIs, so I think it may be worth to move toward removing it from this API as that usage is mostly internal.

GregoryTravis · 2025-02-17T20:58:29Z

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

@@ -199,8 +195,7 @@ type DB_Table
       Arguments:
       - index: The index of the row to get within the table.
       - if_missing: The value to use if the selector isn't present.
-    get_row : Integer -> Any -> Any
-    get_row self index:Integer=0 ~if_missing=Nothing =
+    get_row self index:Integer=0 ~if_missing:Any=Nothing -> Any =


Suggested change

get_row self index:Integer=0 ~if_missing:Any=Nothing -> Any =

get_row self index:Integer=0 ~if_missing:Any=Nothing -> Row ! Index_Out_Of_Bounds =

I realize that this PR isn't for completely fixing all signatures, I'm just pointing out a few obvious existing problems.

No but thanks for pointing them out, probably worth fixing them up if we can.

Just double checked and the correct signature is

Suggested change

get_row self index:Integer=0 ~if_missing:Any=Nothing -> Any =

get_row self index:Integer=0 ~if_missing:Any=Nothing -> Row|Any =

We are not throwing Index_Out_Of_Bounds here but returning if_missing. Because if_missing can be Any, then the type must be Row | Any because we don't know the type of if_missing.

If we had polymorphic types the type could be more precise by making:

Suggested change

get_row self index:Integer=0 ~if_missing:Any=Nothing -> Any =

get_row self index:Integer=0 ~if_missing:'a=Nothing -> Row | 'a =

This calls last_row which can throw Index_Out_Of_Bounds -- do we not include transitive throws here?

Oh so you've found a bug 😅 I'd probably get it fixed in separate PR to keep this PR mostly pure refactor only.

To be honest I'm not sure if we have consistent guidelines on it. In many cases the list of possible errors is not exhaustive. We probably should revisit that but I guess we may want to keep it to only most common errors for now, as listing all may sometimes be huge. Also there's no checking of that currently.

Re (1) - the bug is - get_row should return if_missing (Nothing by default) if called with -1 on an empty table, but you are right that it is actually raising an error. That is an implementation bug, because we do the -1 check before the bounds check. We should rearrange the code to do bounds check first and return if_missing if out-of-bounds, then the last_row call would be guaranteed to not fail.

We seem to have almost no tests for get_row at present.

Reported as #12317

Since these return exceptions aren't checked at all then we shouldn't worry about it too much now; if we implement that checking, that will find all of our omissions 😄

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

radeusgd self-assigned this Feb 14, 2025

radeusgd force-pushed the wip/radeusgd/check-table-column-methods branch from b0a2843 to f295430 Compare February 14, 2025 08:19

Akirathan mentioned this pull request Feb 14, 2025

API check for Standard.Table #12285

Draft

5 tasks

radeusgd force-pushed the wip/radeusgd/check-table-column-methods branch 2 times, most recently from da8626b to d661aab Compare February 17, 2025 12:42

radeusgd added 24 commits February 17, 2025 19:26

comparisons

e6fa9ca

operations 1

c7fb171

operations 2

72589e8

table

071dd34

typos

98153cb

adapt signatures

d13726c

update tests

75c3973

more removing old sigs

6c92253

update test

fae28bd

Revert "update test"

2d40143

This reverts commit 3a393f1.

update test v2

2fff82a

temporarily avoid type error on text contains etc.

0e5366b

update DB_Column using linter

8fb40a8

add element types to checked Vector/Dictionary sigs

c5f7083

more aligning

f067f64

align DB_Table

7b35eb9

fix typo

709a72f

rewrite Table using new rules

943943c

fix whitespace in DB_Table

b13fc5e

fix whitespace in DB_Column

5adec91

fix indentation in doc comment

0745ba2

apply formatting to Column

927b913

fmt

ecc1e66

trying to fix signatures

e42effc

for now make types of Column text ops less strict - inner checks hand…

9cfd48f

…le that anyway reporting as Invalid_Value_Type

radeusgd force-pushed the wip/radeusgd/check-table-column-methods branch from d661aab to 9cfd48f Compare February 17, 2025 18:26

radeusgd added 3 commits February 17, 2025 19:28

Array needed to be accepted in is_in make expressions work

2d03daf

update type signature that was very obsolete, also update obsolete co…

f13080d

…mments

write can return other types than File (e.g. Enso_File, S3_File etc.)

409e81e

radeusgd marked this pull request as ready for review February 17, 2025 19:31

radeusgd requested review from jdunkerley, GregoryTravis, AdRiley and marthasharkey as code owners February 17, 2025 19:31

GregoryTravis reviewed Feb 17, 2025

View reviewed changes

radeusgd added 5 commits February 18, 2025 13:35

Merge branch 'develop' into wip/radeusgd/check-table-column-methods

00e9384

fix double ! signatures

bfd82f1

CR fix missing !

547bd23

CR fix Column instead of DB_Column

89a963f

CR a bit more precise type for get_row

fbc2966

radeusgd requested a review from GregoryTravis February 18, 2025 15:28

GregoryTravis approved these changes Feb 18, 2025

View reviewed changes

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

mergify bot merged commit 59c82f3 into develop Feb 19, 2025
45 of 48 checks passed

mergify bot deleted the wip/radeusgd/check-table-column-methods branch February 19, 2025 12:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use checked signatures for Table and Column #12283

Use checked signatures for Table and Column #12283

radeusgd commented Feb 14, 2025 •

edited

Loading

GregoryTravis Feb 17, 2025

radeusgd Feb 18, 2025

GregoryTravis Feb 18, 2025

radeusgd Feb 18, 2025

GregoryTravis Feb 17, 2025

GregoryTravis Feb 17, 2025

radeusgd Feb 18, 2025

radeusgd Feb 18, 2025

GregoryTravis Feb 18, 2025

radeusgd Feb 18, 2025

radeusgd Feb 18, 2025

radeusgd Feb 18, 2025

GregoryTravis Feb 18, 2025

	get_row self index:Integer=0 ~if_missing:Any=Nothing -> Any =
	get_row self index:Integer=0 ~if_missing:Any=Nothing -> Row ! Index_Out_Of_Bounds =

Use checked signatures for Table and Column #12283

Use checked signatures for Table and Column #12283

Conversation

radeusgd commented Feb 14, 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

radeusgd commented Feb 14, 2025 •

edited

Loading