Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Improvement-9665][python] add create table regex #9673

Merged
merged 5 commits into from
Apr 25, 2022

Conversation

hiSandog
Copy link
Contributor

@hiSandog hiSandog commented Apr 22, 2022

At this stage, creating a table cannot use SQL Task through python-gate.It will be recognized as "SELECT" type, which will cause the task to fail.

So, add create table regex.

This pr will close #9665

@SbloodyS SbloodyS added the first time contributor First-time contributor label Apr 22, 2022
@zhongjiajie
Copy link
Member

I will take a look tomorrow

@zhongjiajie
Copy link
Member

BTW, i think your point on issues is great

Sql type will be identified by the regex. I think it should be specified by a parameter.

We need to implement the override of user-specified sql type parameters, because the user may have specific user case. and we should add param in

self.sql_type: Optional[str] = "select",

and add the overwrite logic in function if self.sql_type is sprcific

@property
def sql_type(self) -> int:
"""Judgement sql type, use regexp to check which type of the sql is."""
pattern_select_str = (
"^(?!(.* |)insert |(.* |)delete |(.* |)drop |(.* |)update |(.* |)alter ).*"
)
pattern_select = re.compile(pattern_select_str, re.IGNORECASE)
if pattern_select.match(self.sql) is None:
return SqlType.NOT_SELECT
else:
return SqlType.SELECT

@zhongjiajie zhongjiajie added bug Something isn't working Python labels Apr 23, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 23, 2022

Codecov Report

Merging #9673 (da08071) into dev (b564e58) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##                dev    #9673      +/-   ##
============================================
- Coverage     40.00%   40.00%   -0.01%     
+ Complexity     4473     4472       -1     
============================================
  Files           835      835              
  Lines         33549    33550       +1     
  Branches       3709     3710       +1     
============================================
  Hits          13421    13421              
+ Misses        18882    18880       -2     
- Partials       1246     1249       +3     
Impacted Files Coverage Δ
...er/master/dispatch/host/assign/RandomSelector.java 77.77% <0.00%> (-5.56%) ⬇️
...e/dolphinscheduler/remote/NettyRemotingClient.java 50.70% <0.00%> (-1.41%) ⬇️
...server/master/runner/task/CommonTaskProcessor.java 4.34% <0.00%> (-0.20%) ⬇️
.../org/apache/dolphinscheduler/api/enums/Status.java 100.00% <0.00%> (ø)
...nscheduler/service/process/ProcessServiceImpl.java 30.54% <0.00%> (ø)
...uler/api/controller/ProcessInstanceController.java 72.09% <0.00%> (ø)
...er/api/controller/ProcessDefinitionController.java 45.34% <0.00%> (ø)
...r/plugin/task/sqoop/parameter/SqoopParameters.java 53.33% <0.00%> (ø)
...er/server/master/runner/WorkflowExecuteThread.java 8.28% <0.00%> (+0.03%) ⬆️
...org/apache/dolphinscheduler/remote/utils/Host.java 40.00% <0.00%> (+2.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b564e58...da08071. Read the comment docs.

@zhongjiajie
Copy link
Member

zhongjiajie commented Apr 23, 2022

BTW, ci failed with python lint error

./src/pydolphinscheduler/tasks/sql.py:81:1: D400 First line should end with a period

TL;TR
run command tox -e lint or flake8 . could run the lint locally

and we have the way about run your code locally which you could see in https://github.com/apache/dolphinscheduler/blob/dev/dolphinscheduler-python/pydolphinscheduler/DEVELOP.md#test-your-code

@sonarcloud
Copy link

sonarcloud bot commented Apr 24, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Member

@zhongjiajie zhongjiajie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks and good catch

@zhongjiajie zhongjiajie merged commit 8a8b63c into apache:dev Apr 25, 2022
@zhongjiajie
Copy link
Member

Thanks @hiSandog for the contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working first time contributor First-time contributor Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement][python] sql type can not be specified
4 participants