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.36k stars 323 forks source link

JSON visualization adds `get` nodes for objects that are not JSON but just have a JSON vis #9606

Closed radeusgd closed 4 months ago

radeusgd commented 7 months ago

image

As can be easily seen on the picture.

The problem is the (super cool feature) of adding get accessor nodes in the JSON visualization is enabled for all types.

In Enso most (all?) types have a JSON visualization based on their to_json method (that has a default implementation provided by Any). But most types do not support a get method that will allow to get fields from this JSON. It only really makes sense for actuaj JS_Object values.

Proposed solutions

I think we should either:

  1. Restrict the click-to-add-node feature to specific types, mostly JS_Object.
  2. or allow it everywhere, but for types other than JS_Object the created nodes should begin with calling to_js_object on the original value (that's what to_json calls in the visualization).
    • _(It's also fine having it on JS_Object too because it is just identity for it, but it would be annoying adding this unnecessary call everywhere.)_
somebody1234 commented 7 months ago

i think approach 1 is cleaner from an API point of view, but i think we should be going for approach 2 because things should Just Work for an end user. we can always figure out an API to communicate to the GUI what the actual expression is for getting that specific property.

kazcw commented 7 months ago

The GUI technically has the type information to make this distinction already, but I think rather than use that it would be a cleaner solution for the visualization to return information about how to access fields alongside the data; this would also generalize better toward what you're suggesting, @somebody1234 --- that sounds really cool and would make the JSON viz a general interface for exploring typed structured data.

Right now the viz just returns the raw JSON; we would need something like:

interface JsonVizData {
  /** JSON */
  data: unknown,
  /** If provided, when creating accessors, a method with this name will be called first (this would probably always be `to_js_object`). */
  to_json: string | null
}

Of course if we implement type-specific getters in the future this would replace the to_json property.

radeusgd commented 7 months ago

The GUI technically has the type information to make this distinction already, but I think rather than use that it would be a cleaner solution for the visualization to return information about how to access fields alongside the data; this would also generalize better toward what you're suggesting, @somebody1234 --- that sounds really cool and would make the JSON viz a general interface for exploring typed structured data.

Right now the viz just returns the raw JSON; we would need something like:

interface JsonVizData {
  /** JSON */
  data: unknown,
  /** If provided, when creating accessors, a method with this name will be called first (this would probably always be `to_js_object`). */
  to_json: string | null
}

Of course if we implement type-specific getters in the future this would replace the to_json property.

This is nice, but I think if we want to go down this path, it may make sense to make it even more generic.

For example, for the case I showed in the ticket with the Date type, when I click on year now I get my_node.get "year" which does not work. With your proposal I'd get my_node.to_js_object.get "year" which works so it is better, but it is a bit going around. Ideally, it should do just my_node.year which is the 'preferred' way to access this field.


To implement that, maybe we need to define some way on the type to declare how to access each of the properties, i.e. a function taking the name of the property and returning a 'code template' to get it. We could probably rely on Meta to implement a generic way that will work for most objects, with the exception of special handling for some (e.g. JS_Object, maybe Map etc.).

kazcw commented 7 months ago

I would think that information would need to be provided as part of the to_json API, since that's where the mapping between fields and JSON fields is established. Anyway, whether we will take the JsonVizData baby step or go directly to a protocol for specifying type-specific getters, the GUI needs more than the raw data from the visualization.

somebody1234 commented 7 months ago

fwiw i'm not sure about providing it in to_json itself - remember, this language also has a textual format, and to_json is the intuitive way imo to generate a json to output into a file.

i think it'd be a better idea to have a separate, related method - something like to_json_with_metadata (an optional parameter to to_json would work but i think it would clutter up the graph editor when the method is shown). it should probably also be marked as internal or something, so it doesn't show up in the Component Browser as well

(edit: note that the above assumes we do end up going the to_json (or similar) route, of course)

farmaazon commented 7 months ago

i think it'd be a better idea to have a separate, related method - something like to_json_with_metadata (an optional parameter to to_json would work but i think it would clutter up the graph editor when the method is shown). it should probably also be marked as internal or something, so it doesn't show up in the Component Browser as well

It won't clutter the Graph Editor, as it will be called as visualization preprocessor, and the returned information should be handled by visualization itself.

somebody1234 commented 7 months ago

@farmaazon i mean an optional parameter on to_json - i can imagine people wanting to explicitly have to_json in their graph

farmaazon commented 4 months ago

Closing in favor of #10246