Attach a warning when Nothing is used in a Filter_Condition #8865
Conversation
GregoryTravis
requested review from
jdunkerley,
radeusgd and
AdRiley
as code owners
distribution/lib/Standard/Table/0.0.0-dev/src/Internal/Filter_Condition_Helpers.enso
Outdated
Show resolved
Hide resolved
distribution/lib/Standard/Table/0.0.0-dev/src/Internal/Filter_Condition_Helpers.enso
Outdated
Show resolved
Hide resolved
| case values of | ||
| _ : Vector -> | ||
| values.fold action (flip (warn_on_nothing_in_comparison filter_condition)) | ||
| _ -> action |
There was a problem hiding this comment.
If a column is passed in, shouldn't we also raise the warning if it contains nulls?
There was a problem hiding this comment.
If the column is costly to compute, are we concerned about the overhead?
There was a problem hiding this comment.
I assume the intention is that we wanted to detect explicit Nothing values used to construct Filter_Conditions, not the presence of Nothing in the data itself.
There was a problem hiding this comment.
I assume the intention is that we wanted to detect explicit
Nothingvalues used to constructFilter_Conditions, not the presence of Nothing in the data itself.
Then I'm not sure if we should warn about the Vector case either.
I think we should be consistent, as otherwise we are building user expectations (I can expect a warning) and then breaking them (If I provide a Column instead of the Vector, there's no warning anymore)
There was a problem hiding this comment.
I think the Vector case is different. That is a direct argument to Filter_Condition.Is_In, and we are warning the user that they are explicitly using a value that cannot match anything. A Column is generally from the user's source-of-truth dataset, and could be very large and often contain Nothing.
My understanding is that we are providing a warning about what was explicitly passed in to a Filter_Condition constructor, not about what the user's dataset might contain.
In addition, scanning the Column for Nothing requires that it be computed, which could be costly.
There was a problem hiding this comment.
Fair, from that point of view the current behaviour seems OK.
| Test.specify "should not attach a warning when comparing with a column containing Nothing in a comparison or `is_in` `Filter_Condition`" <| | ||
| t = table_builder [["x", [1, 2, 3]], ["y", [1, Nothing, 2]]] | ||
| Problems.assume_no_problems (t.filter "x" (Filter_Condition.Equal (t.at "y"))) |
There was a problem hiding this comment.
Again, this test has nothing to do with is_in.
| Problems.assume_no_problems (t.filter "x" (Filter_Condition.Equal [Nothing, Nothing])) | ||
| Problems.assume_no_problems (t.filter "x" (Filter_Condition.Is_In [[Nothing, Nothing]])) | ||
| Problems.assume_no_problems (t.filter "x" (Filter_Condition.Is_In [(t.at "y")])) |
There was a problem hiding this comment.
| Problems.assume_no_problems (t.filter "x" (Filter_Condition.Is_In [(t.at "y")])) | |
| Problems.assume_no_problems (t.filter "x" (Filter_Condition.Is_In (t.at "y"))) |
I think this should go this way. A column inside of a vector is 'too deep' nesting - it would check equality of each value from x if it is equal to the whole column "y", that does not seem right.
GregoryTravis
added
CI: Keep up to date
Pull Request Description
Attach a warning when Nothing is used as a value in a comparison or
is_inFilter_Condition.Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build.