zeppelin icon indicating copy to clipboard operation
zeppelin copied to clipboard

[ZEPPELIN-5896]feat:add sql debug feature

Open zhugezifang opened this issue 2 years ago • 26 comments
trafficstars

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

zhugezifang avatar May 09 '23 09:05 zhugezifang

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 avatar May 09 '23 13:05 zhugezifang

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

zhugezifang avatar May 11 '23 05:05 zhugezifang

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?

Reamer avatar May 11 '23 07:05 Reamer

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

zhugezifang avatar May 11 '23 07:05 zhugezifang

I do not see your changes. grafik

Reamer avatar May 11 '23 08:05 Reamer

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/.

Reamer avatar May 11 '23 08:05 Reamer

I do not see your changes. grafik

the older version

image

the latest version

image

zhugezifang avatar May 11 '23 08:05 zhugezifang

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?

zhugezifang avatar May 11 '23 08:05 zhugezifang

I have created https://github.com/zhugezifang/zeppelin/pull/1 for you. Please look over it. The compilation works. I can't make any statement about the functionality.

Reamer avatar May 11 '23 09:05 Reamer

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

zhugezifang avatar May 11 '23 09:05 zhugezifang

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

yes,it also need

zhugezifang avatar May 12 '23 01:05 zhugezifang

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 avatar May 12 '23 02:05 zhugezifang

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

zhugezifang avatar May 12 '23 07:05 zhugezifang

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 avatar May 12 '23 10:05 zhugezifang

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 avatar May 15 '23 05:05 zhugezifang

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?

zhugezifang avatar May 16 '23 05:05 zhugezifang

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?

jongyoul avatar May 16 '23 15:05 jongyoul

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 avatar May 17 '23 01:05 zhugezifang

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

zhugezifang avatar May 17 '23 01:05 zhugezifang

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 avatar May 18 '23 01:05 zhugezifang

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 avatar May 18 '23 10:05 zhugezifang

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 avatar May 18 '23 13:05 zhugezifang

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

zhugezifang avatar May 23 '23 07:05 zhugezifang

@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 avatar Jun 14 '23 01:06 jongyoul

@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 avatar Jul 04 '23 03:07 zhugezifang

@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?

zhugezifang avatar Jul 13 '23 11:07 zhugezifang