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
Add support for Cockroach DB #9043
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.
First pass comments. Please also add a note that CockroachDB is supported both in /docs/index.rst
(in the bullet list) and /docs/installation.rst
(in the table). You also need to run black
to make sure the code is uniformly formatted.
requirements-dev.txt
Outdated
@@ -16,6 +16,7 @@ | |||
# | |||
black==19.3b0 | |||
coverage==4.5.3 | |||
cockroachdb==0.3.3 |
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.
This needs to be moved to extras_require
in setup.py
.
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.
done
from datetime import datetime | ||
from typing import List, Optional, Tuple, TYPE_CHECKING | ||
|
||
from pytz import _FixedOffset # type: ignore | ||
from sqlalchemy.dialects.postgresql.base import PGInspector | ||
|
||
from superset.db_engine_specs.base import LimitMethod | ||
from superset.db_engine_specs.postgres import PostgresEngineSpec | ||
|
||
if TYPE_CHECKING: | ||
# prevent circular imports | ||
from superset.models.core import Database # pylint: disable=unused-import |
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.
You seem to be importing lots of things that aren't being used in this file. If you check CI logs you will see that this is failing builds.
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.
file is cleaned
Codecov Report
@@ Coverage Diff @@
## master #9043 +/- ##
==========================================
+ Coverage 59.43% 59.45% +0.01%
==========================================
Files 369 369
Lines 11743 11747 +4
Branches 2884 2888 +4
==========================================
+ Hits 6980 6984 +4
Misses 4584 4584
Partials 179 179
Continue to review full report at Codecov.
|
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.
LGTM; did you get everything to work, or are there any caveats?
@derari thanks a lot for your work on this! I tried to apply the changes in this PR manually, and it didn't work OOTB with docker-compose. It seems like adding the cockroach dep to This is first contact with superset for me, so forgive me if this comment is incorrect or misguided! |
CATEGORY
Choose one
SUMMARY
Adds support for Cockroach DB.
I could not test the change becaues the container always fails with
but this seems unrelated to my change.
So far I could use Cockroach DB in superset even without explicit support, only the time grain options were missing.
I verified that the time grain options declared by PostgresEngineSpec work with Cockroach DB.
TEST PLAN
Add a Cockroach DB datasource, add tables and test charts.
ADDITIONAL INFORMATION
REVIEWERS