-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
fix: add kylin physical datasets #20559
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️
We hope to see you in our Slack community too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding engine.table_names() to judge if table_name exists.
Hi@zys2017, thanks for the contribution. There are a couple of database specs here. changing the specific database spec if a database has special behaviors. |
Codecov Report
@@ Coverage Diff @@
## master #20559 +/- ##
===========================================
- Coverage 66.71% 54.98% -11.73%
===========================================
Files 1752 1752
Lines 65478 65478
Branches 6916 6916
===========================================
- Hits 43681 36004 -7677
- Misses 20049 27726 +7677
Partials 1748 1748
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@@ -760,11 +760,11 @@ def get_perm(self) -> str: | |||
|
|||
def has_table(self, table: Table) -> bool: | |||
engine = self.get_sqla_engine() | |||
return engine.has_table(table.table_name, table.schema or None) | |||
return engine.has_table(table.table_name, table.schema or None) or table.table_name in engine.table_names() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am the original Kylinpy maintainer, I just saw the source code of kylinpy. I think you should change here .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Table_names is the function of orm and seems effective to other db source,not only kylin. It can avoid that the engine does not implement relevant functions. kylin may also add the func.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The has_table
is similar to table_names
that should implement in dialect.
Is there any plan to fix this issue in the short term? |
While this issue is being resolved somewhere I suggest everyone who has this problem to implement the has_table method in sqla_dialect.py from kylinpy plugin. You will find it in kylinpy/sqla_dialect.py, and the method is: You should change that return to the next line:
And everything will be back to normal. |
@zys2017 just checking in to see if you (and/or @zhaoyongjie ) have any plans to pick this up, rebase it, and get it across the finish line. |
SUMMARY
Fix the error of kylin physical table add, as these two issues describe:
kylin table in the database cannot be added #19645
Unable to add table from kylin database #17870
ADDITIONAL INFORMATION