[ZEPPELIN-5896]feat:add sql debug feature #4598

zhugezifang · 2023-05-09T09:34:52Z

What is this PR for?

This pull request add sql debug feat into zeppelin

Design Document: https://docs.google.com/document/d/1b0MkUwnPdqXQ3uALpELUUau0D50QEM6z_Xuwrs4GQmA/edit?usp=sharing

What type of PR is it?

Feature

Todos

- Task

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-5896

How should this be tested?

CI

Questions:

Does the license files need to update? No
Is there breaking changes for older versions? No
Does this needs documentation? Yes

Reamer

Is there a reason why you open so many pull requests with the same subject? You can always change the existing pull request. If everything works with the Maven plugin, the pull request can be shortened to a manageable minimum.

zeppelin-interpreter/pom.xml

zeppelin-interpreter/src/main/antrl4/Sql.g4

zhugezifang · 2023-05-09T13:14:32Z

Is there a reason why you open so many pull requests with the same subject? You can always change the existing pull request. If everything works with the Maven plugin, the pull request can be shortened to a manageable minimum.

ok，sorry，i am not familiar with it， i will try it next times，thanks for your advices

zhugezifang · 2023-05-11T05:01:11Z

hi @Reamer is this pr has other problems？ i really want to add this feature to zeppelin， could you help me complete it？

Reamer · 2023-05-11T07:13:05Z

hi @Reamer is this pr has other problems？ i really want to add this feature to zeppelin， could you help me complete it？

Don't you see the review comments from me?

zhugezifang · 2023-05-11T07:57:51Z

hi @Reamer is this pr has other problems？ i really want to add this feature to zeppelin， could you help me complete it？

Don't you see the review comments from me?

i have changed it according to your advice

Reamer · 2023-05-11T08:08:39Z

I do not see your changes.

Reamer · 2023-05-11T08:11:19Z

Please also remove the default configurations. You can probably also delete most of the files under zeppelin-interpreter/src/main/java/org/apache/zeppelin/antrl4/.

zhugezifang · 2023-05-11T08:12:50Z

I do not see your changes.

the older version

the latest version

zhugezifang · 2023-05-11T08:16:18Z

Please also remove the default configurations. You can probably also delete most of the files under zeppelin-interpreter/src/main/java/org/apache/zeppelin/antrl4/.

Please also remove the default configurations --- this is more detaily？such as give the line of code

hi，if delete the code in zeppelin-interpreter/src/main/java/org/apache/zeppelin/antrl4/ ,the feature cannot run ,really to do it?

Reamer · 2023-05-11T09:22:07Z

I have created zhugezifang#1 for you. Please look over it. The compilation works. I can't make any statement about the functionality.

zeppelin-interpreter/pom.xml

zhugezifang · 2023-05-11T09:58:56Z

I have created zhugezifang#1 for you. Please look over it. The compilation works. I can't make any statement about the functionality.

thank you very much for your guidance，i look over it

Reamer

We need also an integration in the new UI zeppelin-web-angular.

zeppelin-interpreter/src/main/java/org/apache/zeppelin/antlr/SqlSplitVisitor.java

zhugezifang · 2023-05-12T01:54:08Z

We need also an integration in the new UI zeppelin-web-angular.

yes，it also need

zhugezifang · 2023-05-12T02:49:48Z

We need also an integration in the new UI zeppelin-web-angular.

hi @Reamer i am not familiar with the new ui of angular , is there a developer familiar with angular in zeppelin community to complete it？

zhugezifang · 2023-05-12T07:47:50Z

We need also an integration in the new UI zeppelin-web-angular.

hi @Reamer i am not familiar with the new ui of angular , is there a developer familiar with angular in zeppelin community to complete it？

We need also an integration in the new UI zeppelin-web-angular.

i try to completed it

Reamer

I found two minor improvements. Can you correct your formatter. For example, Zeppelin uses an indentation of two spaces.
Furthermore, the use of antlr4 must be included in the license.
Also, it would be nice if you upload screenshots of both frontends in the pull request description.
Glad you've been reading up on the new Angular frontend. In my opinion, this one was always easier than the other. But I'm not a frontend developer and can't really evaluate your code there.

zeppelin-interpreter/src/main/java/org/apache/zeppelin/antlr/SqlSplitVisitor.java

zhugezifang · 2023-05-12T10:43:56Z

I found two minor improvements. Can you correct your formatter. For example, Zeppelin uses an indentation of two spaces. Furthermore, the use of antlr4 must be included in the license. Also, it would be nice if you upload screenshots of both frontends in the pull request description. Glad you've been reading up on the new Angular frontend. In my opinion, this one was always easier than the other. But I'm not a frontend developer and can't really evaluate your code there.

ok，thans for your advices,it really help me very much , i learn a lot from processing the problems of this pr , and i completed it

zhugezifang · 2023-05-15T05:39:10Z

I found two minor improvements. Can you correct your formatter. For example, Zeppelin uses an indentation of two spaces. Furthermore, the use of antlr4 must be included in the license. Also, it would be nice if you upload screenshots of both frontends in the pull request description. Glad you've been reading up on the new Angular frontend. In my opinion, this one was always easier than the other. But I'm not a frontend developer and can't really evaluate your code there.

ok，thans for your advices,it really help me very much , i learn a lot from processing the problems of this pr , and i completed it

@Reamer hi，could you help to review it again？

zhugezifang · 2023-05-16T05:37:07Z

I found two minor improvements. Can you correct your formatter. For example, Zeppelin uses an indentation of two spaces. Furthermore, the use of antlr4 must be included in the license. Also, it would be nice if you upload screenshots of both frontends in the pull request description. Glad you've been reading up on the new Angular frontend. In my opinion, this one was always easier than the other. But I'm not a frontend developer and can't really evaluate your code there.

@Reamer hi, could you help to review it again？ is this pr can be merged?

Reamer

I have added a few comments. Please remember to include images of both front ends in the PullRequest description.

zeppelin-interpreter/src/test/java/org/apache/zeppelin/antlr/SqlSplitVisitorTest.java

zeppelin-interpreter/src/main/java/org/apache/zeppelin/antlr/SqlSplitVisitor.java

zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java

zeppelin-web-angular/src/app/pages/workspace/notebook/paragraph/control/control.component.ts

zeppelin-web-angular/src/app/pages/workspace/notebook/paragraph/paragraph.component.ts

jongyoul · 2023-05-16T15:19:42Z

Thank you for your contribution. I already saw some comments by @Reamer but I also could see some style issue. (Yes, Zeppelin doesn't have very-very strict rules for styling but I think contributors should do their best to keep the style with the current one. Some Java code doesn't match with the current code so I recommend you reformat your code block in your IDE. Could you please do it kindly?

zhugezifang · 2023-05-17T01:47:16Z

Thank you for your contribution. I already saw some comments by @Reamer but I also could see some style issue. (Yes, Zeppelin doesn't have very-very strict rules for styling but I think contributors should do their best to keep the style with the current one. Some Java code doesn't match with the current code so I recommend you reformat your code block in your IDE. Could you please do it kindly?

no problem，i will keep it ，thanks for your help @Reamer @jongyoul ，it really help me lot

zhugezifang · 2023-05-17T01:54:10Z

I have added a few comments. Please remember to include images of both front ends in the PullRequest description.

hi @Reamer ，i not do the new ui ，You see, okay？ i am really not familiar with the angular

Reamer

I am very unhappy with the code quality and therefore will not pursue this feature. Another reviewer is welcome to ask my opinion once the code is clean.
In my opinion, most of the code belongs in the JDBC interpreter and not in the general Zeppelin interpreter.
There should also be an implementation for the new frontend, since the old frontend will be replaced at some point.
In the backend will not check for a JDBC paragraph. The check takes place only in the frontend....
@zhugezifang
I don't have the time to develop this feature.

Reamer · 2023-05-17T14:09:05Z

zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java

+    text="";
+    for (String createTableSql:createTableList){
+      text+=createTableSql+"\n";
+      size++;


Why do you iterate through a list to determine the length of the list?

Reamer · 2023-05-17T14:09:59Z

zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java

+    //split
+    List<String> createTableList= SqlSplitVisitor.splitSql(text,paragraphId);
+    int size=0;
+    text="";


Why do you overwrite text? It is better to use a new name here.

Reamer · 2023-05-17T14:10:26Z

zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java

+    int size=0;
+    text="";
+    for (String createTableSql:createTableList){
+      text+=createTableSql+"\n";


Have you ever heard of the StringBuilder?

Reamer · 2023-05-17T14:11:24Z

zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java

+                throws IOException {    
+              super.onSuccess(p, context);
+              //change result
+              setResult(p, finalSize,createTableList);


Why is it necessary to change the result of the paragraph?

Reamer · 2023-05-17T14:12:20Z

zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java

+                connectionManager.unicastParagraph(p.getNote(), p2, context.getAutheInfo().getUser(), fromMessage.msgId);
+              }
+
+              // if it's the last paragraph and not empty, let's add a new one


You have copied a lot of code from other places. This should be prevented.

zhugezifang · 2023-05-18T01:54:32Z

I am very unhappy with the code quality and therefore will not pursue this feature. Another reviewer is welcome to ask my opinion once the code is clean. In my opinion, most of the code belongs in the JDBC interpreter and not in the general Zeppelin interpreter. There should also be an implementation for the new frontend, since the old frontend will be replaced at some point. In the backend will not check for a JDBC paragraph. The check takes place only in the frontend.... @zhugezifang I don't have the time to develop this feature.

I am very sorry, i will improve the code quality, and i will try to completed the new ui , because i am not familiar with angular,it may cost me lot time, but i will try my best to do it. and you really help me a lot to develop this feature, the Limite of my ability，i complete the code which cannot meet your requirements，it really sorry，i try to improve it .At last, Thank you sincerely for your advice very much @Reamer and @jongyoul

zhugezifang · 2023-05-18T10:47:44Z

I am very unhappy with the code quality and therefore will not pursue this feature. Another reviewer is welcome to ask my opinion once the code is clean. In my opinion, most of the code belongs in the JDBC interpreter and not in the general Zeppelin interpreter. There should also be an implementation for the new frontend, since the old frontend will be replaced at some point. In the backend will not check for a JDBC paragraph. The check takes place only in the frontend.... @zhugezifang I don't have the time to develop this feature.

hi, i try my best to improve the code quality, and completed the new ui ,and add images of both front ends in the PullRequest description

zhugezifang · 2023-05-18T13:11:29Z

Thank you for your contribution. I already saw some comments by @Reamer but I also could see some style issue. (Yes, Zeppelin doesn't have very-very strict rules for styling but I think contributors should do their best to keep the style with the current one. Some Java code doesn't match with the current code so I recommend you reformat your code block in your IDE. Could you please do it kindly?

hi @jongyoul could you help me to review this pr?

zhugezifang · 2023-05-23T07:16:50Z

Thank you for your contribution. I already saw some comments by @Reamer but I also could see some style issue. (Yes, Zeppelin doesn't have very-very strict rules for styling but I think contributors should do their best to keep the style with the current one. Some Java code doesn't match with the current code so I recommend you reformat your code block in your IDE. Could you please do it kindly?

hi @jongyoul could you help me to review this pr?

@jongyoul hi

jongyoul · 2023-06-14T01:00:35Z

@zhugezifang Hello, sorry for the late reply. I glance at your PR and suggest you make a child interpreter like jdbc.debug instead of adding a menu. The debug paragraph is currently only for JDBC, but the menu would be exposed for all kinds of paragraphs. zeppelin-interpreter package and NotebookServer class affect all of interpreters. If you want to make an interpreter-specific feature, I recommend adding a new sub-interpreter. WDYT?

zhugezifang · 2023-07-04T03:35:43Z

@zhugezifang Hello, sorry for the late reply. I glance at your PR and suggest you make a child interpreter like jdbc.debug instead of adding a menu. The debug paragraph is currently only for JDBC, but the menu would be exposed for all kinds of paragraphs. zeppelin-interpreter package and NotebookServer class affect all of interpreters. If you want to make an interpreter-specific feature, I recommend adding a new sub-interpreter. WDYT?

@jongyoul hi，the menu of sql debug is controlled, it only can be viewed and used in jdbc interpreter, it cannot be viewed in other interpreter

zhugezifang · 2023-07-13T11:56:58Z

@zhugezifang Hello, sorry for the late reply. I glance at your PR and suggest you make a child interpreter like jdbc.debug instead of adding a menu. The debug paragraph is currently only for JDBC, but the menu would be exposed for all kinds of paragraphs. zeppelin-interpreter package and NotebookServer class affect all of interpreters. If you want to make an interpreter-specific feature, I recommend adding a new sub-interpreter. WDYT?

@zhugezifang Hello, sorry for the late reply. I glance at your PR and suggest you make a child interpreter like jdbc.debug instead of adding a menu. The debug paragraph is currently only for JDBC, but the menu would be exposed for all kinds of paragraphs. zeppelin-interpreter package and NotebookServer class affect all of interpreters. If you want to make an interpreter-specific feature, I recommend adding a new sub-interpreter. WDYT?

@jongyoul hi，the menu of sql debug is controlled, it only can be viewed and used in jdbc interpreter, it cannot be viewed in other interpreter

@jongyoul hi,could help me to review it?

jongyoul · 2023-07-15T10:50:46Z

zeppelin-web-angular/src/app/pages/workspace/notebook/paragraph/control/control.component.ts

+      },
+      {
+        label: 'Debug',
+        show: this.defaultInterpreterGroup === 'jdbc' ? true : false,


@zhugezifang In my understanding, defaultInterpreterGroup can be changed dynamically if you create a new interpreter from the type of interpreter. E.g. We can have jdbc2 as a jdbc type but it won't work properly. So that's the reason why I don't think we can control the type of interpreters. Moreover, we can change the type of paragraph dynamically from jdbc to spark and flink. In this case will it work properly? The third, I don't know if this debug feature is general for all kinds of jdbc-related engine. We can use several types of jdbc drivers by default but I don't know how we can guarantee current jdbc provide jdbc features.

In another aspect, I feel like adding a new interpreter seems quite good. Do you have any concerns to add a new interpreter in the jdbc interpreter group?

Reamer requested changes May 9, 2023

View reviewed changes

zeppelin-interpreter/pom.xml Show resolved Hide resolved

zeppelin-interpreter/pom.xml Outdated Show resolved Hide resolved

zeppelin-interpreter/src/main/antrl4/Sql.g4 Outdated Show resolved Hide resolved

zhugezifang force-pushed the sql_debug_dev branch from e84b056 to bf8d7db Compare May 9, 2023 13:36

Reamer reviewed May 11, 2023

View reviewed changes

zeppelin-interpreter/pom.xml Outdated Show resolved Hide resolved

zhugezifang force-pushed the sql_debug_dev branch from 48dc626 to 029e898 Compare May 11, 2023 13:15

Reamer requested changes May 11, 2023

View reviewed changes

zeppelin-interpreter/src/main/java/org/apache/zeppelin/antlr/SqlSplitVisitor.java Outdated Show resolved Hide resolved

zhugezifang force-pushed the sql_debug_dev branch from 83bb3d1 to 28c6ea8 Compare May 12, 2023 02:19

zhugezifang force-pushed the sql_debug_dev branch from 5e89610 to d9edf18 Compare May 12, 2023 07:47

Reamer requested changes May 12, 2023

View reviewed changes

zeppelin-interpreter/src/main/java/org/apache/zeppelin/antlr/SqlSplitVisitor.java Outdated Show resolved Hide resolved

zeppelin-interpreter/src/main/java/org/apache/zeppelin/antlr/SqlSplitVisitor.java Outdated Show resolved Hide resolved

Reamer reviewed May 12, 2023

View reviewed changes

zeppelin-interpreter/src/main/java/org/apache/zeppelin/antlr/SqlSplitVisitor.java Outdated Show resolved Hide resolved

zhugezifang force-pushed the sql_debug_dev branch 2 times, most recently from 57d11e4 to c023a55 Compare May 12, 2023 10:41

Reamer requested changes May 16, 2023

View reviewed changes

zhugezifang force-pushed the sql_debug_dev branch 3 times, most recently from d7934c5 to 79f53db Compare May 17, 2023 09:24

Reamer requested changes May 17, 2023

View reviewed changes

feat:add sql debug feature

6dd002d

zhugezifang force-pushed the sql_debug_dev branch from b1d76df to 6dd002d Compare May 18, 2023 10:42

zhugezifang mentioned this pull request May 26, 2023

feat:improve query efficiency #4606

Open

1 task

zhugezifang mentioned this pull request Jun 8, 2023

[ZEPPELIN-5915]improve query efficiency #4611

Open

1 task

jongyoul reviewed Jul 15, 2023

View reviewed changes

[ZEPPELIN-5896]feat:add sql debug feature #4598

Are you sure you want to change the base?

[ZEPPELIN-5896]feat:add sql debug feature #4598

Conversation

zhugezifang commented May 9, 2023 • edited

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Questions:

Reamer left a comment

Choose a reason for hiding this comment

zhugezifang commented May 9, 2023

zhugezifang commented May 11, 2023

Reamer commented May 11, 2023

zhugezifang commented May 11, 2023 • edited

Reamer commented May 11, 2023

Reamer commented May 11, 2023

zhugezifang commented May 11, 2023

zhugezifang commented May 11, 2023

Reamer commented May 11, 2023

zhugezifang commented May 11, 2023

Reamer left a comment

Choose a reason for hiding this comment

zhugezifang commented May 12, 2023

zhugezifang commented May 12, 2023

zhugezifang commented May 12, 2023

Reamer left a comment

Choose a reason for hiding this comment

zhugezifang commented May 12, 2023

zhugezifang commented May 15, 2023

zhugezifang commented May 16, 2023 • edited

Reamer left a comment

Choose a reason for hiding this comment

jongyoul commented May 16, 2023

zhugezifang commented May 17, 2023

zhugezifang commented May 17, 2023

Reamer left a comment • edited

Choose a reason for hiding this comment

Reamer May 17, 2023

Choose a reason for hiding this comment

Reamer May 17, 2023

Choose a reason for hiding this comment

Reamer May 17, 2023

Choose a reason for hiding this comment

Reamer May 17, 2023

Choose a reason for hiding this comment

Reamer May 17, 2023

Choose a reason for hiding this comment

zhugezifang commented May 18, 2023

zhugezifang commented May 18, 2023

zhugezifang commented May 18, 2023

zhugezifang commented May 23, 2023

jongyoul commented Jun 14, 2023

zhugezifang commented Jul 4, 2023 • edited

zhugezifang commented Jul 13, 2023

jongyoul Jul 15, 2023

Choose a reason for hiding this comment

zhugezifang commented May 9, 2023 •

edited

zhugezifang commented May 11, 2023 •

edited

zhugezifang commented May 16, 2023 •

edited

Reamer left a comment •

edited

zhugezifang commented Jul 4, 2023 •

edited