deephaven-core icon indicating copy to clipboard operation
deephaven-core copied to clipboard

feat: Support `remove_blink` to remove blink table semantics in server-side Python

Open alexpeters1208 opened this issue 1 year ago • 2 comments

removeBlink is used by the Java API to disable the specialized aggregation semantics that blink tables implement. This wrapper adds the functionality to disable those semantics from the Python API.

alexpeters1208 avatar Aug 19 '24 21:08 alexpeters1208

My understanding of a blink-table is a refreshing one that discards all the previous-cycle-added rows in the current cycle. Does remove_blink change this behavior? If not, then the name is 'misleading'.

jmao-denver avatar Aug 22 '24 20:08 jmao-denver

I spoke with @jmao-denver at length about this naming / behavior, and I want to summarize that discussion here.

It seems that there are three possible competing definitions of "blink table":

  1. t is blink table if and only if t obeys blink aggregation semantics Per this definition, if t does not obey blink aggregation semantics, it is not a blink table. So, calling remove_blink to disable those semantics means that the result is not a blink table, even though it appears to still be blinking.

  2. t is a blink table if and only if t obeys blink aggregation semantics and t dispenses of all of its existing rows every update cycle Per this definition, if t fails to satisfy either property, it is not a blink table. So again, calling remove_blink results in a non-blink table.

  3. t is a blink table if and only if t dispenses of all of its existing rows every update cycle Per this definition, calling remove_blink results in another blink table without specialized aggregation semantics. Any table that dispenses of all of its rows every update cycle is a blink table, regardless of how it aggregates.

Now, here's the difficulty / confusion: The existing code and naming suggests that the engine is employing the first definition. IE, calling removeBlink on the underlying Java table sets the isBlink attribute to false. Anyone reading this code would conclude that the result is not a blink table.

I would argue that the third definition is the most intuitive. It seems that the key property of a "blink" table would be the "blinking" property, not how such a table handles aggregations. In fact, I didn't know about these specialized aggregation semantics until very recently.

@jmao-denver called me to discuss confusion over this, and we agreed that while definitions 2 or 3 might make sense, defintion 1 does not. One possible partial solution is to consider renaming remove_blink to something like standard_aggs, no_mem, no_agg, remove_blink_aggs, no_blink_agg, etc. This does not do anything to address the apparent tension in definitions of "blink", but maybe if we don't expose an API that uses those words, we don't have to talk about that.

alexpeters1208 avatar Aug 22 '24 21:08 alexpeters1208

There is a lot of discussion above, but it seems to be overkill. The Javadocs are fairly straightforward: https://deephaven.io/core/javadoc/io/deephaven/engine/table/Table.html#removeBlink()

If this table is a blink table, i.e. it has [BLINK_TABLE_ATTRIBUTE](https://deephaven.io/core/javadoc/io/deephaven/engine/table/Table.html#BLINK_TABLE_ATTRIBUTE) set to true, return a child without the attribute, restoring standard semantics for aggregation operations.
Returns:
A non-blink child table, or this table if it is not a blink table

Is there any reason to make this PR more complex than the javadocs?

chipkent avatar Aug 23 '24 19:08 chipkent

Labels indicate documentation is required. Issues for documentation have been opened:

Community: https://github.com/deephaven/deephaven-docs-community/issues/303

deephaven-internal avatar Sep 04 '24 19:09 deephaven-internal