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

Autoscoping for Table filter #9277

Closed JaroslavTulach closed 7 months ago

JaroslavTulach commented 8 months ago

The Table filter method uses

@filter Widget_Helpers.make_filter_condition_selector

make sure the filter argument can use autoscoped constructors in the IDE.

### Tasks
- [ ] Modify `make_filter_condition_selector` to use _autoscoped constructors_
- [ ] Test the behavior of the IDE remains the same
- [ ] Decide whether a fix needs to be done on the language server side or the IDE
JaroslavTulach commented 8 months ago

The IDE probably need to handle .. prefix for autoscoped constructors as prefix and not operator with missing lhs first to actually test the Table.filter behavior.

4e6 commented 8 months ago

gui interprets the expression ..Constructor as a binary operation with the first argument not provided. The work on the parser is needed to recognize .. as a unary operator applied to constructor.

enso-autoscope-filter

4e6 commented 8 months ago

Also, the expression updates for autoscoped constructor calls don't contain the method pointer. The work on the engine side is also needed.

4e6 commented 8 months ago

The issue is that during the instrumentation, the result of the ..Constructor call is an UnresolvedConstructor object. To create a method pointer, the engine needs to intercept a node after the method resolution is done. \cc @JaroslavTulach

JaroslavTulach commented 8 months ago

The issue is that during the instrumentation, the result of the ..Constructor call is an UnresolvedConstructor object.

I see. We cannot create Method Call object from UnresolvedConstructor as Adam requested...

To create a method pointer, the engine needs to intercept a node after the method resolution is done.

However an UnresolvedConstructor gets converted to real constructor according to docs at the end.

enso-bot[bot] commented 8 months ago

Dmitry Bushev reports a new STANDUP for yesterday (2024-03-11):

Progress: Started working on the task. Investigated how the autoscope constructors with arguments are displayed in the gui. Investigated how the autoscope calls are represented during the instrumentation. It should be finished by 2024-03-13.

Next Day: Next day I will be working on the #9277 task. Continue working on the task

4e6 commented 8 months ago

It seems that all the problems with autoscope are from the fact that the resolution is happening in runtime. The ..Constructor is supposed to mean "I'm too lazy to write the type name, you can figure it out yourself". And we indeed should be able to do that in the compile time. Given

type T
    A
    B
type S
    B
foo t:T|S = t

This will require a kind of type inference (i.e. associating types with expressions) which the compiler cannot do right now. But I think it's a good opportunity to start by implementing this very isolated scenario \cc @radeusgd

Also, making the name resolution in compile time will not affect the runtime performance.

JaroslavTulach commented 8 months ago

... autoscope ... resolution is happening in runtime. ... be able to do that in the compile time.

The possibility of having autoscoping based on static type inference has been discussed and rejected before the work started. If you believe static type inference is the only way to fix the problems for Table.filter, please continue the discussion in its discussion forum.

Let's leave this issue focused on solving the problem, but feel free to question the overall concept.

enso-bot[bot] commented 8 months ago

Dmitry Bushev reports a new STANDUP for yesterday (2024-03-12):

Progress: Continue working on the task. Investigated the issue of why the method calls are not reported for the autoscope constructors. It should be finished by 2024-03-13.

Next Day: Next day I will be working on the #9277 task. Continue working on the task

hubertp commented 7 months ago

Should probably be closed once #9259 is closed.

JaroslavTulach commented 7 months ago

With commit https://github.com/enso-org/enso/pull/9452/commits/3a986f04fbaee7d30bab6261fbefaf88ed452faa from #9452 I was able to get operator51546.filter 'Organisation' filter=(..Equal 'SpaceX') working with following change in Table:

diff --git distribution/lib/Standard/Table/0.0.0-dev/src/Data/Table.enso distribution/lib/Standard/Table/0.0.0-dev/src/Data/Table.enso
index a42bc1dd8c..9aaf724a4f 100644
--- distribution/lib/Standard/Table/0.0.0-dev/src/Data/Table.enso
+++ distribution/lib/Standard/Table/0.0.0-dev/src/Data/Table.enso
@@ -1476,16 +1476,17 @@ type Table
     filter self column (filter : Filter_Condition | (Any -> Boolean) = Filter_Condition.Equal True) on_problems=Report_Warning = case column of
         _ : Column ->
             mask filter_column = Table.Value (self.java_table.filter filter_column.java_column)
-            case filter of
+            f = Panic.catch Any (filter:Filter_Condition) _->filter
+            case f of
                 _ : Filter_Condition ->
-                    resolved = (self:Table_Ref).resolve_condition filter
+                    resolved = (self:Table_Ref).resolve_condition f
                     mask (make_filter_column column resolved on_problems)
                 _ : Function -> Filter_Condition_Module.handle_constructor_missing_arguments filter <|
                     mask (column.map filter)
-        _ : Expression -> self.filter (self.evaluate_expression column on_problems) filter on_problems
+        _ : Expression -> @Tail_Call self.filter (self.evaluate_expression column on_problems) filter on_problems
         _ ->
             table_at = self.at column
-            self.filter table_at filter on_problems
+            @Tail_Call self.filter table_at filter on_problems

e.g. the inline type ascription (filter : Filter_Condition | (Any -> Boolean) is quite problematic. It is the same as (filter : Filter_Condition | Function) and autoscoped constructor has type Function. E.g. it matches the runtime type without any conversion! Moreover doing case filter of and _ : Filter_Condition pattern matching doesn't force the conversion of UnresolvedConstructor to real Filter_Condition - it just doesn't match. Hence the f = Panic.catch Any (filter:Filter_Condition) _->filter attempt to force a conversion. Nasty.

IDE knows Equal's argument

However the IDE seems to know ..Equal takes one argument and what kind of values it can offer for that argument.

radeusgd commented 7 months ago

e.g. the inline type ascription (filter : Filter_Condition | (Any -> Boolean) is quite problematic. It is the same as (filter : Filter_Condition | Function) and autoscoped constructor has type Function. E.g. it matches the runtime type without any conversion! Moreover doing case filter of and _ : Filter_Condition pattern matching doesn't force the conversion of UnresolvedConstructor to real Filter_Condition - it just doesn't match. Hence the f = Panic.catch Any (filter:Filter_Condition) _->filter attempt to force a conversion. Nasty.

Huh, that's an interesting edge case! The filter operator taking a function is indeed complicating things a bit - we also need special handling for to more nicely display info about missing args to Filter_Condition (see: unify_condition_or_predicate). I guess we could move this special 'hack' into unify_condition_or_predicate as well.

Hopefully it will not be needed in too many places - fortunately I don't think there's many other places that mix an autoscoped type with Function.

4e6 commented 7 months ago

https://github.com/enso-org/enso/pull/9293#issuecomment-2035445131 verifies that after fixing the expression updates for autoscope constructors in #9452, the dropdowns with autoscope constructors work in gui the same way as normal constructors