reata / sqllineage

SQL Lineage Analysis Tool powered by Python
MIT License
1.19k stars 215 forks source link

Set param from config #545

Closed delphisharp closed 5 months ago

delphisharp commented 5 months ago

Dear ALL:

目前模型的默认模式名是通过环境变量传递的,我认为实现方法不够python。 所以,我实现了另一种解决方法。

  1. 修改 sqllineage/config.py 实现配置项的读取和写入, 读取方法保持和已有方法兼容。
  2. 修改 sqllineage/core/models.py 的Schema 类,在每次初始化的时候动态读取配置
  3. 修改 sqllineage/runner.py的init方法,传入变量,并在 SQLLineageConfig 赋值 请大家提出意见,谢谢

Currently the default schema name of the model is passed through environment variables, and I think the implementation method is not python enough. So, I implemented another workaround.

  1. Modify sqllineage/config.py to implement reading and writing of configuration items. The reading method remains compatible with the existing method.
  2. Modify the Schema class of sqllineage/core/models.py and dynamically read the configuration every time it is initialized.
  3. Modify the init method of sqllineage/runner.py, pass in variables, and assign values in SQLLineageConfig Please give your opinions, thank you
delphisharp commented 5 months ago

已重新提交代码。 本地测试通过。 Code has been resubmitted. The local pytest passed.

delphisharp commented 5 months ago

Due to code format reasons, resubmit.

codecov[bot] commented 5 months ago

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (0823bf2) 99.50% compared to head (d49cdb0) 99.10%. Report is 1 commits behind head on master.

:exclamation: Current head d49cdb0 differs from pull request most recent head c904f09. Consider uploading reports for the commit c904f09 to get more accurate results

Files Patch % Lines
sqllineage/config.py 80.48% 8 Missing :warning:
sqllineage/core/models.py 80.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #545 +/- ## ========================================== - Coverage 99.50% 99.10% -0.40% ========================================== Files 41 41 Lines 2201 2239 +38 ========================================== + Hits 2190 2219 +29 - Misses 11 20 +9 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

maoxingda commented 5 months ago

https://github.com/maoxingda/sqllineage/blob/0823bf2e32f19d0f64a2c7acb951213db8696b93/sqllineage/config.py#L19

我感觉你这个改动有点问题呀,你现在的写法不支持环境变量的配置了。

I feel there is something wrong with your change. Your current writing method does not support the configuration of environment variables.

https://github.com/reata/sqllineage/pull/540

我正在通过环境变量的方式配置是否启用 列别名引用功能

I am configuring whether to enable the lateral column alias reference function through environment variables.

reata commented 5 months ago

https://github.com/maoxingda/sqllineage/blob/0823bf2e32f19d0f64a2c7acb951213db8696b93/sqllineage/config.py#L19

我感觉你这个改动有点问题呀,你现在的写法不支持环境变量的配置了。

I feel there is something wrong with your change. Your current writing method does not support the configuration of environment variables.

540

我正在通过环境变量的方式配置是否启用 列别名引用功能

I am configuring whether to enable the lateral column alias reference function through environment variables.

Agreed, we need to maintain support config through environment variables. This approach is read only.

The new ability we're adding through this PR is to set config at run time via Python code.

delphisharp commented 5 months ago

刚才在添加测试用例,各位意见现在看到了。我总结下:

  1. 优先走环境变量
  2. 单独从配置类中读取
  3. 参数不通过Runner传递,通过设定 config类实现

I was adding test cases just now, and your comments are now visible. Let me summarize:

  1. Prioritize environment variables
  2. Read from the configuration from config Class
  3. The parameters are not passed through the Runner, set config class
delphisharp commented 5 months ago

Fix the issue and commit the code. The unit test passes.