Make fill_nothing take an empty string

[AdRiley]

Pull Request Description

I think fill_nothing should work with an empty string. Currently TextType.preciseTypeForValue(arg.asString()) throws if you give it an empty string so this fix avoid that.

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.

[radeusgd]

I think an important edge case is unhandled, and I suspect the operation will crash if filling a fixed-length string column with "".

Please add a test for this case to ensure it works correctly.

[AdRiley]

I'm wondering if fill_nothing shouldn't be changing types at all.

[radeusgd]

I'm wondering if fill_nothing shouldn't be changing types at all.

I assume the alternative is to ensure the default value fits the target type, right?

i.e. for a column (Value_Type.Char size=3 variable_length=False), a value "a" or "" would by disallowed, only strings of exactly length 3 would be allowed? And we could return an error message that suggests that if you want to have a different size, you should cast to variable_length=True first?

[AdRiley]

I assume the alternative is to ensure the default value fits the target type, right?

Yes and to error if it doesn't

i.e. for a column (Value_Type.Char size=3 variable_length=False), a value "a" or "" would by disallowed, only strings of exactly length 3 would be allowed? And we could return an error message that suggests that if you want to have a different size, you should cast to variable_length=True first?

Yes but fixed length strings can still hold strings smaller than their size. Right? At least that's how they work in the db world. The fixed/variable size is to do with how they are stored not the data in them.

[radeusgd]

i.e. for a column (Value_Type.Char size=3 variable_length=False), a value "a" or "" would by disallowed, only strings of exactly length 3 would be allowed? And we could return an error message that suggests that if you want to have a different size, you should cast to variable_length=True first?

Yes but fixed length strings can still hold strings smaller than their size. Right? At least that's how they work in the db world. The fixed/variable size is to do with how they are stored not the data in them.

At least currently in Enso, fixed length means that all string values within that column have this fixed length. Currently, regardless of the type, all strings are stored in the same way (I guess that could change with followups of #8512).

In DBs, shorter strings (in fixed length column) are usually padded, right?

[AdRiley]

In DBs, shorter strings (in fixed length column) are usually padded, right?

I think from the users/functionality point of view then no they are not.

https://www.postgresql.org/docs/current/datatype-character.html

`CREATE TABLE test1 (a character(4));
INSERT INTO test1 VALUES ('ok');
SELECT a, char_length(a) FROM test1; -- (1)

a | char_length
------+-------------
ok | 2

CREATE TABLE test2 (b varchar(5));
INSERT INTO test2 VALUES ('ok');
INSERT INTO test2 VALUES ('good ');
INSERT INTO test2 VALUES ('too long');
ERROR: value too long for type character varying(5)
INSERT INTO test2 VALUES ('too long'::varchar(5)); -- explicit truncation
SELECT b, char_length(b) FROM test2;

b | char_length
-------+-------------
ok | 2
good | 5
too l | 5`

But if we are talking about how they are stored on disk then yes they are likely padded.

Again from https://www.postgresql.org/docs/current/datatype-character.html
"Values of type character are physically padded with spaces to the specified width n, and are stored and displayed that way. However, trailing spaces are treated as semantically insignificant and disregarded when comparing two values of type character."

At least currently in Enso, fixed length means that all string values within that column have this fixed length. Currently, regardless of the type, all strings are stored in the same way (I guess that could change with followups of #8512).

I think this will cause us problems if we try to round trip fixed width fields from and to a database. I think we should change this behaviour but not as part of this MR. For now I'm going to add the tests you suggested and see what happens for this fix.

PostgreSQL Documentation

8.3. Character Types

8.3. Character Types # Table 8.4. Character Types Name Description character varying(n), varchar(n) variable-length with limit character(n), char(n), bpchar(n) fixed-length, blank-padded bpchar variable …

[AdRiley]

I'm wondering if fill_nothing shouldn't be changing types at all.

I've changed my mind. I like the type expanding where needed.

[radeusgd]

Do you think we could check this invariant for Postgres?

[AdRiley]

I wasn't sure how to specify "don't test this on SQLite". Do we have anywhere else we do this?

[radeusgd]

1. One way is to check the prefix which contains the name of the DB: https://github.com/enso-org/enso/blob/develop/test/Table_Tests/src/Common_Table_Operations/Filter_Spec.enso#L85 But I think that's not quite perfect.

2. The preferred way is to use 'feature flags' from test selection, and I think setup.test_selection.fixed_length_text_columns is exactly what is needed here:
   https://github.com/enso-org/enso/blob/develop/test/Table_Tests/src/Common_Table_Operations/Conversion_Spec.enso#L76

[AdRiley]

Thank you

[radeusgd]

IMHO while this type becomes variable length, it could retain a known max-size:

Suggested change

	actual.at "col0" . value_type . should_equal (Value_Type.Char variable_length=True)
	actual.at "col0" . value_type . should_equal (Value_Type.Char size=3 variable_length=True)

what do you think?

[AdRiley]

Yes that would be better wouldn't it? I'll look what that code change would be.

[radeusgd]

Two small test suggestions that I'm not 100% convinced are needed.
Overall it looks OK.

[AdRiley]

I'm going to merge this one and resolve the 2 suggestions in a second PR as they have some bigger implications that we should discuss.