Wrap the Table slice op #3110
Wrap the Table slice op #3110
Conversation
| with self.assertRaises(DHError): | ||
| rt = t.slice(3, 2) |
There was a problem hiding this comment.
Is this really the behavior we want? It seems like we may want to just return an empty table. In fact, if an index is negative, that is what we do.
There was a problem hiding this comment.
The Java doc is very explicit about this behavior. It does seem inconsistent though but I would rather have an exception than an empty table which would take up some resource and would not be of any use. @rcaudy your thoughts?
There was a problem hiding this comment.
I'm not particularly interested in re-litigating the semantics of this operation while wrapping from Python, and I'm not unhappy with the current behavior.
We throw an exception for non-sensical inputs in 2 scenarios:
firstPositionInclusive < 0 && lastPositionExclusive > 0: This is a range counted from the end for first and from the beginning for last.- Both positive or both negative and
lastPositionExclusive < firstPositionInclusive: This is an "inverted" range that can never be non-empty.
We could just call these impossible ranges "empty", but that seems user-hostile in (likely, IMO) cases where the user made a mistake in preparing their inputs. In the current approach, they can choose to handle the exception.
The "exceptional case" is that we always allow:
firstPositionInclusive >= 0 && lastPositionExclusive < 0: This is a range defined relative to the end and the beginning, but not necessarily an empty range. This is just as valid as ranges larger than the current table size in either direction, and the output is only empty if there aren't currently any rows in it.
There was a problem hiding this comment.
There is no relitigation going on. The API was never discussed with me in the first place. It is inconsistent with other data science software.
If you look at Python and NumPy, both gracefully handle all cases. They both return the empty case when the indexing range is a null set.
Matlab / Octave behaves the same as Python / NumPy and also returns the empty case.
Q takes a slightly different tact. It returns null values when indexing outside of the range. Yet Q still gracefully returns a result.
https://code.kx.com/q4m3/3_Lists/#343-indexing-domain
There was a problem hiding this comment.
I wouldn't call giving someone a useless result when they almost certainly screwed up their inputs "graceful". It's also just strictly worse when you imagine a scenario with a ticking parent table; the user could proceed to set up a whole scaffold of child operations that wait forever for data that can never change. This is the kind of thing enterprise users hate us for when we implement them the way you're proposing.
Regardless, if you want to discuss changing this, it can be a separate issue targeting the query engine; the Python wrapper should stay very thin here.
There was a problem hiding this comment.
This is similar to TableTools.merge with no non-null inputs. We throw an exception, rather than giving the users an empty result that will never, ever tick.
There was a problem hiding this comment.
I agree that the Python wrapper should have the same semantics as calling from Java, and I like the exception for inverted ranges, but I do think we should support the x[-7:8] case to be consistent with other software.
Returning empty for inverted ranges I think sets you up for confusion...returning null I could almost get behind, except I think in practice for us it just sets you up for an NPE down the road. Like if it were slicing an array in the query language, I'd say we should return null for those cases (since other functions down the road would handle the null), but if slicing a table, you really need a non-null result for it to be useful anyway.
There was a problem hiding this comment.
I agree with Raffi's position as stated above. I do see it as reasonable to support a start relative to end and a last relative to start.
There was a problem hiding this comment.
I agree with Raffi.
- I've never really viewed it as a benefit that Python returns an empty object when you give it bad indices to slice.
- I have always viewed it as a benefit to be able to slice relative to the end of an object.
I don't think throwing an error when incorrectly attempting to slice a table will detract from the user experience, so long as the error makes it obvious.
|
Labels indicate documentation is required. Issues for documentation have been opened: How-to: https://github.com/deephaven/deephaven.io/issues/1917 |
Fixes #3089