enso-org / enso

Enso Analytics is a self-service data prep and analysis platform designed for data teams.
https://ensoanalytics.com
Apache License 2.0
7.38k stars 323 forks source link

Inconsistent warning propagation in Vector #11570

Open radeusgd opened 1 day ago

radeusgd commented 1 day ago

I'm not 100% sure if this is not intended behaviour, but I don't think it is correct.

Let's see the following code:

from Standard.Base import all

foo v =
    if v.length == 0 then "empty" else "not empty"

summarize name v =
    ws = Warning.get_all v . map .value
    IO.println "Value "+name+"="+v.to_display_text+'; warnings='+ws.to_text

type My_Warning
    Value msg

    to_display_text self -> Text = "My_Warning: "+self.msg
    to_text self -> Text = self.to_display_text

main =
    one = Warning.attach (My_Warning.Value "ONE") 1
    two = Warning.attach (My_Warning.Value "TWO") 2
    v = [one, two]
    summarize "v" v
    b1 = foo v
    summarize "b1" b1
    summarize "(v.at 0)" (v.at 0)

    p = Pair.new one two
    summarize "p" p
    b2 = foo p
    summarize "b2" b2
    summarize "(p.at 0)" (p.at 0)

Actual behaviour

I'm skipping the warnings being printed by the instrument as they are rather redundant. I wanted to have full control of displaying warnings on our values so I relied on good old println.

Value v=[1, 2]; warnings=[My_Warning: TWO, My_Warning: ONE]
Value b1=not empty; warnings=[]
Value (v.at 0)=1; warnings=[My_Warning: TWO, My_Warning: ONE]
Value p=Pair(1, 2); warnings=[My_Warning: TWO, My_Warning: ONE]
Value b2=not empty; warnings=[My_Warning: TWO, My_Warning: ONE]
Value (p.at 0)=1; warnings=[My_Warning: TWO, My_Warning: ONE]

Expected behaviour

IMO b1 should also have the 2 warnings attached, just like all the other values in this example.

I don't know if we have a clear specification for this, but I assumed that we had agreed on it that the Vector shouldn't really be special and it should propagate the warnings in the same way as any Atom does. From user's perspective both Pair and Vector are just a collection and the warnings should be propagated the same way.

If I check warnings on the vector v I get the 2 warnings. So if I then have a value that depends on v (by dataflow), it should inherit these warnings.

Interestingly, v.at 0 will actually inherit both warnings. It is correct, but moreover shows that the warnings are no longer part of the elements, they were already propagated to the whole vector and any operation depending on the vector should inherit them too.

radeusgd commented 1 day ago

I also checked the Dictionary behavior:

from Standard.Base import all

foo v =
    if v.length == 0 then "empty" else "not empty"

summarize name v =
    ws = Warning.get_all v . map .value
    IO.println "Value "+name+"="+v.to_display_text+'; warnings='+ws.to_text

type My_Warning
    Value msg

    to_display_text self -> Text = "My_Warning: "+self.msg
    to_text self -> Text = self.to_display_text

main =
    one = Warning.attach (My_Warning.Value "ONE") 1
    two = Warning.attach (My_Warning.Value "TWO") 2
    v = [one, two]
    summarize "v" v
    b1 = foo v
    summarize "b1" b1
    summarize "(v.at 0)" (v.at 0)

    p = Pair.new one two
    summarize "p" p
    b2 = foo p
    summarize "b2" b2
    summarize "(p.at 0)" (p.at 0)

    m = Dictionary.from_vector [["jeden", one], ["dwa", two]]
    summarize "m" m
    b3 = foo m
    summarize "b3" b3
    summarize '(m.at "jeden")' (m.at "jeden")

It yields:

Value v=[1, 2]; warnings=[My_Warning: TWO, My_Warning: ONE]
Value b1=not empty; warnings=[]
Value (v.at 0)=1; warnings=[My_Warning: TWO, My_Warning: ONE]
Value p=Pair(1, 2); warnings=[My_Warning: TWO, My_Warning: ONE]
Value b2=not empty; warnings=[My_Warning: TWO, My_Warning: ONE]
Value (p.at 0)=1; warnings=[My_Warning: TWO, My_Warning: ONE]
Value m=Dictionary; warnings=[My_Warning: TWO, My_Warning: ONE]
Value b3=not empty; warnings=[My_Warning: TWO, My_Warning: ONE]
Value (m.at "jeden")=1; warnings=[My_Warning: TWO, My_Warning: ONE]

So it seems that all kinds of collections, apart from Vector correctly propagate the warnings. This reinforces the point that indeed the behaviour of Vector should be amended - I really don't see why it should behave differently.

radeusgd commented 1 day ago

As @GregoryTravis mentioned we may also want to check the behaviour for Array as well. We probably should add tests based on the above repro to ensure that Vector, Array, Dictionary and Pair all behave consistently in the scenario above.