Implement relational NULL semantics for Nothing for in-memory Column operations #8816

GregoryTravis · 2024-01-19T20:52:09Z

Pull Request Description

Updates in-memory table column operations to treat Nothing as a relational NULL.
This PR does not include changes to Table.join.

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.

GregoryTravis · 2024-01-19T20:57:35Z

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

@@ -1600,7 +1600,7 @@ type Column
                run_vectorized_binary_op self op_name as_vector expected_result_type=Value_Type.Boolean skip_nulls=False new_name=result_name
            False ->
                set = Set.from_vector as_vector error_on_duplicates=False
-                run_unary_op self set.contains new_name=result_name skip_nulls=False expected_result_type=Value_Type.Boolean
+                run_unary_op self set.contains_relational new_name=result_name skip_nulls=True expected_result_type=Value_Type.Boolean


@radeusgd I'm not sure how to invoke the fallback implementation of == (line 214) -- what case uses this?

I need more context for this question. Line 214 of which file?

Line 214 of this file -- I can't leave a comment there.
Or here: https://github.com/enso-org/enso/blob/develop/distribution/lib/Standard/Table/0.0.0-dev/src/Data/Column.enso#L214

I think this code might need to be modified for handling nulls differently, but I'm not sure what cases use it.

Try using == on a Mixed column, e.g.

t = Table.new [["X", ["a", 42, Nothing]], ["Y", ["b", 42, True]]] (t.at "X") == (t.at "Y")

I think it should trigger it, but you can add some println to make sure :)

Ah, I remember now -- == uses skip_nulls=True, so the fallback is never used for nulls. So no change is needed.

radeusgd · 2024-01-22T13:25:13Z

std-bits/table/src/main/java/org/enso/table/data/column/operation/map/SpecializedIsInOp.java

+      if (storage.isNa(i)) {
+        missing.set(i);


What if arg.size() == 0? Looking at the contains_relational semantics, it should result in False, but looking at this code it seems that it will result in Nothing.

Do we have a test case for such scenario? If not, please let's add it and ensure the implementation is correct.

radeusgd · 2024-01-22T13:33:29Z

std-bits/table/src/main/java/org/enso/table/util/ImmutableBitSet.java

+/**
+ * A wrapper around BitSet that implements boolean operations conveniently. Unlike BitSet,
+ * ImmutableBitSet takes a size parameter, which allows .not to be implemented.
+ */


radeusgd · 2024-01-22T13:36:31Z

test/Table_Tests/src/Common_Table_Operations/Nothing_Spec.enso

+        # [input, had_null, had_true, had_false, output]
+        cases = [[True, True, True, True, True]]
+            + [[True, True, True, False, True]]
+            + [[True, False, True, True, True]]
+            + [[True, False, True, False, True]]
+            + [[True, True, False, True, Nothing]]
+            + [[True, True, False, False, Nothing]]
+            + [[True, False, False, True, False]]
+            + [[True, False, False, False, False]]
+            + [[False, True, True, True, True]]
+            + [[False, True, False, True, True]]
+            + [[False, False, True, True, True]]
+            + [[False, False, False, True, True]]
+            + [[False, True, True, False, Nothing]]
+            + [[False, True, False, False, Nothing]]
+            + [[False, False, True, False, False]]
+            + [[False, False, False, False, False]]
+            + [[Nothing, True, True, True, Nothing]]
+            + [[Nothing, True, False, True, Nothing]]
+            + [[Nothing, False, True, True, Nothing]]
+            + [[Nothing, False, False, True, Nothing]]
+            + [[Nothing, True, True, False, Nothing]]
+            + [[Nothing, True, False, False, Nothing]]
+            + [[Nothing, False, True, False, Nothing]]
+            + [[Nothing, False, False, False, False]]
+


IMO we should generate this programmatically, it is very hard to parse and check this huge table for correctness.

AdRiley · 2024-01-23T10:27:50Z

std-bits/table/src/main/java/org/enso/table/data/column/operation/map/bool/BooleanIsInOp.java

@@ -60,43 +60,30 @@ public Storage<?> runZip(
  }

  private BoolStorage run(BoolStorage storage, boolean hadNull, boolean hadTrue, boolean hadFalse) {


Perhaps just my lack of knowledge of the codebase, but what is the difference between Null, Nothing and Missing? This PR is about Nothing, this function takes a hadNull and generates a newMissing.

I think they all mean the same concept here really, so it is just a bit of chaos that got into the codebase over time.

Nothing is the Enso-centrict name.

Null probably came from the Java internals naming convention (where the Nothing is often represented as a null reference).

missing is the naming that stemmed from also old version of Table (we used to have Column.is_missing instead of Column.is_nothing; the external API got renamed, but the internals likely stayed. The *Missing bitsets are used when we are storing values like integers in unboxed long[] arrays - such don't take null (like Object[] could), so there needs to be a separate flag of which values are missing. I guess it could be called isNothing though, for consistency.

We could probably rename all of these to nothing-based names.

AdRiley · 2024-01-23T11:06:02Z

std-bits/table/src/main/java/org/enso/table/util/ImmutableBitSet.java

+  }
+
+  public static ImmutableBitSet allFalse(int size) {
+    return new ImmutableBitSet(new BitSet(), size);


Is

return new ImmutableBitSet(new BitSet(size), size);

going to make the allocation more efficient?

Looking at the usages, I think to the contrary.

The thing is that new BitSet() creates an empty bitset, which will answer false to any bitset.get(ix).

So it creates the allFalse bitset while using the minimal amount of memory. Whereas specifying the size, will pre-allocate the memory - which in this case just seems unnecessary.

So actually I think that keeping not adding the size is best for allocation (but I'm only 85% sure about it).

Yes, BitSet is really a set of 1-positions, not a set of boolean values, which is why I needed ImmutableBitSet to have a size field, so it would really be more like a set of bits. Specifically, you can't use .not() unless you know what the last bit position is. (The not-related methods are the only ones that use size for something other than checking that the size are the same.)

AdRiley · 2024-01-23T11:06:37Z

std-bits/table/src/main/java/org/enso/table/util/ImmutableBitSet.java

+  }
+
+  public static ImmutableBitSet allTrue(int size) {
+    return new ImmutableBitSet(new BitSet(), size).not();


Similarly

return new ImmutableBitSet(new BitSet(size), size).not(); ?

radeusgd · 2024-01-23T12:15:02Z

test/Table_Tests/src/Common_Table_Operations/Nothing_Spec.enso

+            neg_input = negation_case.at 1
+            neg_argument = negation_case.at 2


nitpick but IMO this is misleading neg_input may be negate but it may as well be identity. So the neg_ is misleading what this is.

Maybe just transform_input instead?

Suggested change

neg_input = negation_case.at 1

neg_argument = negation_case.at 2

transform_input = negation_case.at 1

transform_argument = negation_case.at 2

GregoryTravis added 30 commits January 9, 2024 13:21

empty

32e5781

==

d76d2ab

Merge branch 'develop' into wip/gmt/5156-nothing

8b5d2d1

comparisons

f7b18e9

is_nothing

9b67c34

column

2543611

between

0e6422b

is_in

85ca4bf

not

f4e89ec

distinct, remove js_object

9a7b047

order_by

bba6daa

Merge branch 'develop' into wip/gmt/5156-nothing

987c366

rename to comparisons

e35bfda

add Nothing value

ad4bada

skip comparables

9ef1d05

Merge branch 'develop' into wip/gmt/5156-nothing

1118fa2

Two more is_in cases, no mixed columns in db test

42b7762

cast columns for db backends

ef83197

no date_time for sqlite

0a62d1d

remove special null handling for is_in Vector

9046cba

remove special null handling for is_in Column

49c54e6

fix time zone test failures in postgres

db5634f

is_in test with no nothings in the col

1c94d88

Fix Char==Nothing non-fallback case

6f53139

slow bool is_in, full test cases

8416e0b

four negation cases

10ef070

optimized

ae8c4c6

andNot

573de61

ttf comments

e923c7a

all is_in but Mixed

8425998

GregoryTravis added 4 commits January 19, 2024 15:14

fix integer_overflow_spec

305c018

fix Codegen_Spec

fd40de3

fix Codegen_Spec

564587d

java fmt

391a421

GregoryTravis commented Jan 19, 2024

View reviewed changes

GregoryTravis marked this pull request as ready for review January 19, 2024 20:58

GregoryTravis requested review from jdunkerley, radeusgd and AdRiley as code owners January 19, 2024 20:58

changelog

b4db21b

radeusgd reviewed Jan 22, 2024

View reviewed changes

GregoryTravis added 3 commits January 22, 2024 12:21

Handle null vs empty in general is_in

83645ba

Handle null vs empty in general is_in, only check once

b692bea

generate cases

0d328cf

GregoryTravis added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Jan 22, 2024

GregoryTravis requested a review from radeusgd January 22, 2024 18:45

GregoryTravis added 2 commits January 22, 2024 14:00

Merge branch 'develop' into wip/gmt/5156-nothing

d5280bd

fix changelog format

7f610d0

AdRiley reviewed Jan 23, 2024

View reviewed changes

radeusgd reviewed Jan 23, 2024

View reviewed changes

radeusgd approved these changes Jan 23, 2024

View reviewed changes

jdunkerley approved these changes Jan 23, 2024

View reviewed changes

AdRiley approved these changes Jan 23, 2024

View reviewed changes

GregoryTravis added 2 commits January 24, 2024 10:56

Merge branch 'develop' into wip/gmt/5156-nothing

ac11b26

rename neg_ vars

2f77775

GregoryTravis added the CI: Ready to merge This PR is eligible for automatic merge label Jan 24, 2024

mergify bot merged commit 5eb3f3b into develop Jan 24, 2024
26 of 27 checks passed

mergify bot deleted the wip/gmt/5156-nothing branch January 24, 2024 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement relational NULL semantics for Nothing for in-memory Column operations #8816

Implement relational NULL semantics for Nothing for in-memory Column operations #8816

GregoryTravis commented Jan 19, 2024 •

edited

Loading

GregoryTravis Jan 19, 2024

radeusgd Jan 22, 2024

GregoryTravis Jan 22, 2024

radeusgd Jan 22, 2024

GregoryTravis Jan 22, 2024 •

edited

Loading

radeusgd Jan 23, 2024

radeusgd Jan 22, 2024

GregoryTravis Jan 22, 2024

radeusgd Jan 22, 2024

radeusgd Jan 22, 2024

GregoryTravis Jan 22, 2024

AdRiley Jan 23, 2024

radeusgd Jan 23, 2024

AdRiley Jan 23, 2024

radeusgd Jan 23, 2024

GregoryTravis Jan 24, 2024 •

edited

Loading

AdRiley Jan 23, 2024

radeusgd Jan 23, 2024

GregoryTravis Jan 24, 2024

		@@ -60,43 +60,30 @@ public Storage<?> runZip(
		}

		private BoolStorage run(BoolStorage storage, boolean hadNull, boolean hadTrue, boolean hadFalse) {

		neg_input = negation_case.at 1
		neg_argument = negation_case.at 2

Implement relational NULL semantics for Nothing for in-memory Column operations #8816

Implement relational NULL semantics for Nothing for in-memory Column operations #8816

Conversation

GregoryTravis commented Jan 19, 2024 • edited Loading

Pull Request Description

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

GregoryTravis Jan 22, 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

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

GregoryTravis Jan 24, 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

GregoryTravis commented Jan 19, 2024 •

edited

Loading

GregoryTravis Jan 22, 2024 •

edited

Loading

GregoryTravis Jan 24, 2024 •

edited

Loading