Database Visualizations #1335

radeusgd · 2021-03-16T21:36:18Z

Pull Request Description

Implements visualizations for the Database library:

the Table visualization is enhanced to be able to automatically materialize database tables (so now it supports both dataframes and databases).
- It also limits the output to at most 1000 elements and displays the count of hidden elements below the table.
the SQL visualization that pretty prints the query with interpolated values.

Important Notes

This PR should be merged after the backing changes, that is PR Database Visualization Support enso#1582 (and Fix Table Visualization When No Index Is Present enso#1595) is released so that the IDE can see these changes within the engine that it is using.

Checklist

Please include the following checklist in your PR:

The CHANGELOG.md was updated with the changes introduced in this PR.
The documentation has been updated if necessary.
All code conforms to the Rust style guide.
All code has automatic tests where possible.
All code has been profiled where possible.
All code has been manually tested in the IDE.
All code has been manually tested in the "debug/interface" scene.
All code has been manually tested by the PR owner against our test scenarios.
All code has been manually tested by at least one reviewer against our test scenarios.

radeusgd · 2021-03-16T21:41:08Z

Below we can see an example with some filtering applied to the table. The Table visualization displays the materialized results:

Alternatively we can switch to the SQL visualization which shows the query that will be used to compute the results:

The interpolated values that come from Enso are highlighted and their color is based on their type (currently only basic types are supported and that is inferred from their Enso type, not from SQL - we have access to the SQL type so maybe we should switch to that - the only issue is that the representation of SQL types is based on implementation-defined numeric identifiers for each type which I didn't want to hardcode, but if we want to switch to this, I could approximate the datatype name on the backend side and send it as text to the visualization).

Note for clarity: the jdbc: prefix when opening a DB connection is not necessary, skipping it will also work.

wdanilo · 2021-03-17T08:31:57Z

Looks cool. Let's have a video call with @sylwiabr to see how it works and to chat about missing things. Great job with that!

wdanilo · 2021-03-19T09:05:29Z

We are waiting for this to be finished.

wdanilo · 2021-03-24T10:24:13Z

src/rust/ide/view/graph-editor/src/builtin/visualization/java_script/sql.js

+loadScript('https://cdnjs.cloudflare.com/ajax/libs/sql-formatter/4.0.2/sql-formatter.min.js')
+
+class SqlVisualization extends Visualization {
+    static inputType = 'Any' // 'Standard.Database.Data.Table.Table | Standard.Database.Data.Column.Column' // TODO change this once sum types are properly supported


Too long line, max 100 chars everywhere - apply to ALL files

In comments always add links to issues describing them.

Comments should be valid english sentences. missing dot

Apply to all your code.

wdanilo · 2021-03-24T10:24:59Z

src/rust/ide/view/graph-editor/src/builtin/visualization/java_script/sql.js

+        this.setPreprocessorModule('Standard.Visualization.Sql.Visualization')
+        this.setPreprocessorCode(`x -> here.prepare_visualization x`)
+
+        // mock theme


comments should be valid sentences. Also, they should be understandable by people not familiar with codebase, thisdefinitely is not. It needs to describe why it is a mock and what issue it is waiting for with a link to the issue.

This mock theme will be removed anyway before the PR is merged, as we discussed that this PR should be merged after #1337

src/rust/ide/view/graph-editor/src/builtin/visualization/java_script/sql.js

wdanilo · 2021-03-24T10:27:05Z

src/rust/ide/view/graph-editor/src/builtin/visualization/java_script/sql.js

+        function splitTypeName(name) {
+            var ix = name.lastIndexOf('.')
+            if (ix < 0) {
+                return ['', name]
+            }
+
+            return [name.substr(0, ix + 1), name.substr(ix + 1)]
+        }


all these functions should have comments describing their behavior. Or better naming. If I see name splitTypeName I do not understand what does it do - split from what, into what? Better naming is preferred. IF its not possible, musch better and detailed docs. Thesame applies to other functions, like a super-long function rendeParameter. Its namedoesnt tell anything about what it does, and is looks complex.

Added comments and changed names a bit.

wdanilo · 2021-03-24T10:28:13Z

src/rust/ide/view/graph-editor/src/builtin/visualization/java_script/sql.js

+
+            const actualTypeColor = theme.getColorForType(actualType)
+            const fgColor = actualTypeColor
+            let bgColor = [...fgColor.slice(0, 3), bgAlpha]


magic numbers. Do not use literals like that. Extract them to well named variables. You can use 0 as it means start, but 3 is magical

Ok, I changed this to const baseChannels = 3 since that 3 represents the RGB channels that are kept when changing opacity; it's also now a separate function so it is easier to see what it does.

I assume that I can use 2 without treating it like a magic number when I'm dividing things by two, right? (Like I want to compute the middle of something, so I have width / 2)

src/rust/ide/view/graph-editor/src/builtin/visualization/java_script/sql.js

wdanilo · 2021-03-24T10:32:15Z

I'm not sure regarding the code quality - the functions are super long, lack of docs in many places, long expressions are not split into variables etc. I would like @farmaazon to review it from a perspective of someone that does not know what this code does and leave comments - this code should be understandable to every reader easily, without browisng connected code - the logic should be easy to follow. I feel it is not. Ada, would you mind looking at it please?

radeusgd · 2021-03-24T10:35:09Z

I'm not sure regarding the code quality - the functions are super long, lack of docs in many places, long expressions are not split into variables etc. I would like @farmaazon to review it from a perspective of someone that does not know what this code does and leave comments - this code should be understandable to every reader easily, without browisng connected code - the logic should be easy to follow. I feel it is not. Ada, would you mind looking at it please?

I think I could refactor it a bit to make the functions a bit smaller and add docs before a re-review?

radeusgd · 2021-03-24T14:18:15Z

Some screenshots on what the visualizations look like:

radeusgd · 2021-03-24T18:34:07Z

Thanks to #1337 we now have proper colors!

wdanilo

This code is MUCH nicer. I was able to create a proper review of it now! Few inor things, Im accepting it. After fixing them, feel free to merge it <3

CHANGELOG.md

src/rust/ide/view/graph-editor/src/builtin/visualization/java_script/sql.js

wdanilo · 2021-03-25T01:11:08Z

src/rust/ide/view/graph-editor/src/builtin/visualization/java_script/sql.js

+            const formatted = sqlFormatter.format(parsedData.code, {
+                params: params,
+                language: language,
+            })


this line would probably fail if parsedData is for example undefined, null, or a number. We should make our visualizations as error prone as possible, so please, make more checks regarding the input data!

Well, parsedData is what is returned by the visualization preprocessor which is controlled by the visualization itself, so I think we have control over what parsedData should look like.

src/rust/ide/view/graph-editor/src/builtin/visualization/java_script/sql.js

which was committed by mistake

(rounded corners, highlighted index)

Original commit: enso-org/ide@132ae3e

radeusgd added Difficulty: Core Contributor Should only be attempted by a core contributor Priority: High Should be scheduled as soon as possible Type: Enhancement An enhancement to the current state of Enso IDE Category: Visualizations Visualizations embedded in Enso IDE labels Mar 16, 2021

radeusgd self-assigned this Mar 16, 2021

radeusgd force-pushed the wip/rw/database-visualizations branch from dd2254b to 66d4094 Compare March 18, 2021 16:20

radeusgd marked this pull request as ready for review March 18, 2021 22:51

radeusgd requested a review from wdanilo as a code owner March 18, 2021 22:51

wdanilo marked this pull request as draft March 19, 2021 09:05

radeusgd force-pushed the wip/rw/database-visualizations branch 2 times, most recently from 090dd8c to ca98458 Compare March 23, 2021 16:38

radeusgd marked this pull request as ready for review March 23, 2021 22:45

wdanilo reviewed Mar 24, 2021

View reviewed changes

radeusgd force-pushed the wip/rw/database-visualizations branch 2 times, most recently from 13c2f7b to 9849cd7 Compare March 24, 2021 13:56

radeusgd requested a review from wdanilo March 24, 2021 14:12

radeusgd force-pushed the wip/rw/database-visualizations branch from 05b2eb2 to 117ba73 Compare March 24, 2021 18:32

wdanilo approved these changes Mar 25, 2021

View reviewed changes

radeusgd added 5 commits March 25, 2021 14:17

Updated Table visualization

9810338

Update geomap imports

98ec26d

Fix Table style

39678ac

Implement SQL visualization

e556707

Change quotes in SQL vis, run prettier

ed42239

radeusgd added 13 commits March 25, 2021 14:17

Implement type inference hints for Sql visualization

a3e2943

Cosmetic improvements to the SQL vis

eaef600

Add index to Table visualization

e11b64f

Fix JSON vis in Table

0e9885c

revert a change in build/run.js

6f5a285

which was committed by mistake

Improve style for Table

b57b727

(rounded corners, highlighted index)

Improve SQL mismatch selection area

156c135

CHANGELOG, formatting

de3b667

Refactor

959361e

Fix offsets

6047c86

Switch to real themes

7ba5c95

Improve table style

0ed8280

Code review 2

69178f7

radeusgd force-pushed the wip/rw/database-visualizations branch from f159037 to 69178f7 Compare March 25, 2021 14:54

radeusgd merged commit 132ae3e into develop Mar 25, 2021

radeusgd deleted the wip/rw/database-visualizations branch March 25, 2021 16:03

mwu-tow pushed a commit to enso-org/enso that referenced this pull request Oct 30, 2021

Database Visualizations (enso-org/ide#1335)

09080eb

Original commit: enso-org/ide@132ae3e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Database Visualizations #1335

Database Visualizations #1335

radeusgd commented Mar 16, 2021 •

edited

radeusgd commented Mar 16, 2021 •

edited

wdanilo commented Mar 17, 2021

wdanilo commented Mar 19, 2021

wdanilo Mar 24, 2021

wdanilo Mar 24, 2021

radeusgd Mar 24, 2021

wdanilo Mar 24, 2021

radeusgd Mar 24, 2021

wdanilo Mar 24, 2021

radeusgd Mar 24, 2021

wdanilo commented Mar 24, 2021

radeusgd commented Mar 24, 2021

radeusgd commented Mar 24, 2021 •

edited

radeusgd commented Mar 24, 2021

wdanilo left a comment

wdanilo Mar 25, 2021

radeusgd Mar 25, 2021

Database Visualizations #1335

Database Visualizations #1335

Conversation

radeusgd commented Mar 16, 2021 • edited

Pull Request Description

Important Notes

Checklist

radeusgd commented Mar 16, 2021 • edited

wdanilo commented Mar 17, 2021

wdanilo commented Mar 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wdanilo commented Mar 24, 2021

radeusgd commented Mar 24, 2021

radeusgd commented Mar 24, 2021 • edited

radeusgd commented Mar 24, 2021

wdanilo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeusgd commented Mar 16, 2021 •

edited

radeusgd commented Mar 16, 2021 •

edited

radeusgd commented Mar 24, 2021 •

edited