fix: ODPS（MaxCompute）data source table preview failed#32556
fix: ODPS（MaxCompute）data source table preview failed#32556zhutong6688 wants to merge 12 commits intoapache:masterfrom
Conversation
dosubot
bot
added
data:connect
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Fix Detected |
|---|---|---|
 |
Database-specific logic in generic DAO ▹ view | |
|
Tight coupling with concrete engine implementation ▹ view | |
 |
Redundant Column Metadata Fetch ▹ view | |
 |
SQL Injection Risk in Partition Query Construction ▹ view | |
 |
Ineffective Partition Filtering ▹ view | |
 |
Unnecessary Debug Print Statement ▹ view | ✅ |
 |
Undocumented Complex Regex Pattern ▹ view | |
|
Overly Strict ODPS URI Pattern Matching ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| superset/daos/database.py | ✅ |
| superset/db_engine_specs/odps.py | ✅ |
| superset/sql/parse.py | ✅ |
| superset/databases/api.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- On any given comment that Korbit raises on your pull request, you can have a discussion with Korbit by replying to the comment.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Feedback and Support
A print was omitted and deleted during development, and this time it is to delete the print
|
Just assign this task to me, thank you |
sadpandajoe
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #32556 +/- ##
===========================================
+ Coverage 60.48% 83.34% +22.85%
===========================================
Files 1931 550 -1381
Lines 76236 39545 -36691
Branches 8568 0 -8568
===========================================
- Hits 46114 32957 -13157
+ Misses 28017 6588 -21429
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@zhutong6688 Workday happened and I did missed your updates, my mistake! I've assigned this to you and left 1 review. I'll let folks with Oracle knowledge assessing added LoCs. |
|
@justinpark Please kindly agree to proceed with the inspection |
|
@justinpark @hainenber Please approve the execution of the workflow check |
I am extending the support for Alibaba Cloud's MaxCompute data source, also known as ODPS, Not Oracle |
zhutong6688
changed the title
zhutong6688
changed the title
|
@hainenber |
|
The credit is all to @zhutong6688. I'll try my best to assist the author to have the change qualified to be merged. |
|
@hainenber @justinpark I have just resolved the conflict that was detected during the inspection below |
thanks @zhutong6688 @hainenber |
|
@hainenber @justinpark Please approve the execution of the workflow check |
I'm curious about how this case is going. |
|
Looks like CI passed... we just need a qualified reviewer to take a fresh pass at it. |
|
@hainenber @justinpark @mistercrunch anyone feel qualified to give this a proper review and get it merged? |
| # -> https://github.com/aio-libs/async-timeout/blob/master/CHANGES.rst#500-2024-10-31 | ||
| async_timeout>=4.0.0,<5.0.0 | ||
|
|
||
| pyodps>=0.12.2 |
There was a problem hiding this comment.
oh very few people use odps AFAIK, this dependency should be treated like the other analytics databases here: https://github.com/apache/superset/blob/master/pyproject.toml#L160-L188
| return ssh_tunnel | ||
|
|
||
| @classmethod | ||
| def is_odps_partitioned_table( |
There was a problem hiding this comment.
all of the database-engine-specific code belongs in this package -> https://github.com/apache/superset/tree/master/superset/db_engine_specs
I see you you're setting this up as part of your PR, but this odps-specific method/logic belongs in that module.
|
|
||
|
|
||
| @dataclass(eq=True, frozen=True) | ||
| class Partition: |
There was a problem hiding this comment.
this module is getting refactored as we speak by @betodealmeida , he can probably help making sure this lands safely
| # loop over all subclasses of BaseEngineSpec | ||
| for engine in load_engine_specs(): | ||
| if engine is not BaseEngineSpec: | ||
| if ( |
There was a problem hiding this comment.
for unit tests, if they dependencies on external drivers (pyodps in this case) , there's a decorator for conditional unit tests that run only when the dependency is there. Currently we only install some of the database drivers in our CI builds (the ones that end up in development.txt). Most likely we don't want to install all possible drivers and run unit tests against it because there's just too many databases and drivers for the core maintainers to manage... Many drivers are finicky, and some drivers/integrations only few people care about.
Anyhow, not a bad thing to have unit tests in the repo, but won't run as part of our CI. Maybe in your fork/CI you can run tests that are odps-specific.
|
Looks like we have some conflicts and comments that haven't been addressed yet. Setting to draft for now, but we'd love to se this marked "ready for review" and merged! |
|
Bumping this up as the resolution of this PR will help us a lot. |
7 participants
SUMMARY
I fixed an error when using sqllab to preview data after configuring the odps data source in superset, and added partition restrictions for previewing SQL
Unlike before, this is a separate collection of the logic of ODPS into a single class（before #32310）
fixes: #32301
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
This is a preview of the odps table data before the repair：
This is a preview of the repaired ODPS table data：
In addition, when using other data sources, there is no problem with testing and it does not affect other data sources
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION