You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/04/30 11:26:03 UTC
[GitHub] [incubator-superset] dpgaspar opened a new pull request #9702: tests(engine_specs): full bigquery engine coverage
dpgaspar opened a new pull request #9702:
URL: https://github.com/apache/incubator-superset/pull/9702
### CATEGORY
- [ ] Bug Fix
- [ ] Enhancement (new features, refinement)
- [ ] Refactor
- [X] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
Increases BigQuery test coverage to 100%. Took the chance and added more human readable descriptions for this db engines tests
Result coverage:
```
superset/db_engine_specs/athena.py 20 3 85%
superset/db_engine_specs/base.py 344 40 88%
superset/db_engine_specs/bigquery.py 82 0 100%
superset/db_engine_specs/clickhouse.py 16 1 94%
superset/db_engine_specs/cockroachdb.py 3 0 100%
superset/db_engine_specs/db2.py 10 1 90%
superset/db_engine_specs/dremio.py 7 1 86%
superset/db_engine_specs/drill.py 26 5 81%
superset/db_engine_specs/druid.py 34 16 53%
superset/db_engine_specs/elasticsearch.py 16 1 94%
superset/db_engine_specs/exasol.py 10 2 80%
superset/db_engine_specs/gsheets.py 5 0 100%
superset/db_engine_specs/hana.py 18 6 67%
superset/db_engine_specs/hive.py 232 125 46%
superset/db_engine_specs/impala.py 22 4 82%
superset/db_engine_specs/kylin.py 14 1 93%
superset/db_engine_specs/mssql.py 43 0 100%
superset/db_engine_specs/mysql.py 46 21 54%
superset/db_engine_specs/oracle.py 25 3 88%
superset/db_engine_specs/pinot.py 41 17 59%
superset/db_engine_specs/postgres.py 37 0 100%
superset/db_engine_specs/presto.py 435 123 72%
superset/db_engine_specs/redshift.py 7 1 86%
superset/db_engine_specs/snowflake.py 46 10 78%
superset/db_engine_specs/sqlite.py 30 8 73%
superset/db_engine_specs/teradata.py 9 1 89%
superset/db_engine_specs/vertica.py 3 0 100%
```
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
@willbarrett
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull request #9702: tests(engine_specs): full bigquery engine coverage
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9702:
URL: https://github.com/apache/incubator-superset/pull/9702#discussion_r418137845
##########
File path: superset/db_engine_specs/bigquery.py
##########
@@ -174,22 +177,22 @@ def df_to_sql(cls, df: pd.DataFrame, **kwargs: Any) -> None:
`DataFrame.to_gbq()` which requires `pandas_gbq` to be installed.
:param df: Dataframe with data to be uploaded
- :param kwargs: kwargs to be passed to to_gbq() method. Requires both `schema
- and ``name` to be present in kwargs, which are combined and passed to
- `to_gbq()` as `destination_table`.
+ :param kwargs: kwargs to be passed to to_gbq() method. Requires that `schema`,
+ `name` and `con` are present in kwargs. `name` and `schema` are combined
+ and passed to `to_gbq()` as `destination_table`.
"""
try:
import pandas_gbq
from google.oauth2 import service_account
except ImportError:
raise Exception(
Review comment:
While we're here, could we raise something other than `Exception`?
##########
File path: superset/db_engine_specs/bigquery.py
##########
@@ -174,22 +177,22 @@ def df_to_sql(cls, df: pd.DataFrame, **kwargs: Any) -> None:
`DataFrame.to_gbq()` which requires `pandas_gbq` to be installed.
:param df: Dataframe with data to be uploaded
- :param kwargs: kwargs to be passed to to_gbq() method. Requires both `schema
- and ``name` to be present in kwargs, which are combined and passed to
- `to_gbq()` as `destination_table`.
+ :param kwargs: kwargs to be passed to to_gbq() method. Requires that `schema`,
+ `name` and `con` are present in kwargs. `name` and `schema` are combined
+ and passed to `to_gbq()` as `destination_table`.
"""
try:
import pandas_gbq
from google.oauth2 import service_account
except ImportError:
raise Exception(
- "Could not import the library `pandas_gbq`, which is "
+ "Could not import libraries `pandas_gbq` or `google.oauth2`, which are "
"required to be installed in your environment in order "
"to upload data to BigQuery"
)
- if not ("name" in kwargs and "schema" in kwargs):
- raise Exception("name and schema need to be defined in kwargs")
+ if not ("name" in kwargs and "schema" in kwargs and "con" in kwargs):
+ raise Exception("name, schema and con need to be defined in kwargs")
Review comment:
Same here - if we're already in this part of the code, let's alter this to be a custom exception.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request #9702: tests(engine_specs): full bigquery engine coverage
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9702:
URL: https://github.com/apache/incubator-superset/pull/9702#discussion_r417962202
##########
File path: tests/db_engine_specs/bigquery_tests.py
##########
@@ -57,3 +73,112 @@ def test_timegrain_expressions(self):
col=col, pdf=None, time_grain="PT1H", type_=type_
)
self.assertEqual(str(actual), expected)
+
+ def test_fetch_data(self):
+ """
+ DB Eng Specs (bigquery): Test fetch data
+ """
+ # Mock a google.cloud.bigquery.table.Row
+ class Row(object):
+ def __init__(self, value):
+ self._value = value
+
+ def values(self):
+ return self._value
+
+ data1 = [(1, "foo")]
+ with mock.patch.object(BaseEngineSpec, "fetch_data", return_value=data1):
+ result = BigQueryEngineSpec.fetch_data(None, 0)
+ self.assertEqual(result, data1)
+
+ row1 = Row(1)
+ row2 = Row(2)
+ data2 = [row1, row2]
Review comment:
yes, better
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request #9702: tests(engine_specs): full bigquery engine coverage
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9702:
URL: https://github.com/apache/incubator-superset/pull/9702#discussion_r417959566
##########
File path: tests/db_engine_specs/bigquery_tests.py
##########
@@ -33,18 +41,26 @@ def test_bigquery_sqla_column_label(self):
self.assertEqual(actual, expected)
def test_convert_dttm(self):
+ """
+ DB Eng Specs (bigquery): Test conversion to date time
+ """
dttm = self.get_dttm()
test_cases = {
"DATE": "CAST('2019-01-02' AS DATE)",
"DATETIME": "CAST('2019-01-02T03:04:05.678900' AS DATETIME)",
"TIMESTAMP": "CAST('2019-01-02T03:04:05.678900' AS TIMESTAMP)",
+ "TIME": "CAST('03:04:05.678900' AS TIME)",
+ "TIMEDATE": None,
Review comment:
sure
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on pull request #9702: tests(engine_specs): full bigquery engine coverage
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on pull request #9702:
URL: https://github.com/apache/incubator-superset/pull/9702#issuecomment-621969840
Totally agree and was tempted in doing that, but yes scope creep
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request #9702: tests(engine_specs): full bigquery engine coverage
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9702:
URL: https://github.com/apache/incubator-superset/pull/9702#discussion_r417943501
##########
File path: tests/db_engine_specs/bigquery_tests.py
##########
@@ -33,18 +41,26 @@ def test_bigquery_sqla_column_label(self):
self.assertEqual(actual, expected)
def test_convert_dttm(self):
+ """
+ DB Eng Specs (bigquery): Test conversion to date time
+ """
dttm = self.get_dttm()
test_cases = {
"DATE": "CAST('2019-01-02' AS DATE)",
"DATETIME": "CAST('2019-01-02T03:04:05.678900' AS DATETIME)",
"TIMESTAMP": "CAST('2019-01-02T03:04:05.678900' AS TIMESTAMP)",
+ "TIME": "CAST('03:04:05.678900' AS TIME)",
+ "TIMEDATE": None,
Review comment:
I was thrown off for a second, "what type is that?" 😆 Perhaps call it `UNKNOWNTYPE` to make it clear that it's not expected to be found.
##########
File path: tests/db_engine_specs/bigquery_tests.py
##########
@@ -57,3 +73,112 @@ def test_timegrain_expressions(self):
col=col, pdf=None, time_grain="PT1H", type_=type_
)
self.assertEqual(str(actual), expected)
+
+ def test_fetch_data(self):
+ """
+ DB Eng Specs (bigquery): Test fetch data
+ """
+ # Mock a google.cloud.bigquery.table.Row
+ class Row(object):
+ def __init__(self, value):
+ self._value = value
+
+ def values(self):
+ return self._value
+
+ data1 = [(1, "foo")]
+ with mock.patch.object(BaseEngineSpec, "fetch_data", return_value=data1):
+ result = BigQueryEngineSpec.fetch_data(None, 0)
+ self.assertEqual(result, data1)
+
+ row1 = Row(1)
+ row2 = Row(2)
+ data2 = [row1, row2]
Review comment:
Perhaps just throw those on the same row: `data2 = [Row(1), Row(2)]`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org