[GitHub] [incubator-superset] etr2460 commented on a change in pull request #8058: Add docstrings and typing to db_engine_specs and sql_parse
etr2460 commented on a change in pull request #8058: Add docstrings and typing to db_engine_specs and sql_parse URL: https://github.com/apache/incubator-superset/pull/8058#discussion_r316451455 ## File path: superset/db_engine_specs/base.py ## @@ -454,75 +634,93 @@ def select_star( qry = qry.limit(limit) if latest_partition: partition_query = cls.where_latest_partition( -table_name, schema, my_db, qry, columns=cols +table_name, schema, database, qry, columns=cols ) -if partition_query != False: # noqa +if partition_query is not None: qry = partition_query -sql = my_db.compile_sqla_query(qry) +sql = database.compile_sqla_query(qry) if indent: sql = sqlparse.format(sql, reindent=True) return sql @classmethod -def modify_url_for_impersonation(cls, url, impersonate_user, username): +def modify_url_for_impersonation(cls, url, impersonate_user: bool, username: str): Review comment: should add `-> None:` to this 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request #8058: Add docstrings and typing to db_engine_specs and sql_parse
etr2460 commented on a change in pull request #8058: Add docstrings and typing
to db_engine_specs and sql_parse
URL:
https://github.com/apache/incubator-superset/pull/8058#discussion_r316451230
##
File path: superset/db_engine_specs/base.py
##
@@ -386,37 +499,90 @@ def adjust_database_uri(cls, uri, selected_schema):
Some database drivers like presto accept '{catalog}/{schema}' in
the database component of the URL, that can be handled here.
"""
+# TODO: All overrides mutate input uri; should be renamed or refactored
return uri
@classmethod
def patch(cls):
Review comment:
this should be `def patch(cls) -> None:` right?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request #8058: Add docstrings and typing to db_engine_specs and sql_parse
etr2460 commented on a change in pull request #8058: Add docstrings and typing
to db_engine_specs and sql_parse
URL:
https://github.com/apache/incubator-superset/pull/8058#discussion_r316452736
##
File path: superset/db_engine_specs/presto.py
##
@@ -944,30 +955,31 @@ def where_latest_partition(cls, table_name, schema,
database, qry, columns=None)
column_names = {column.get("name") for column in columns or []}
for col_name, value in zip(col_names, values):
if col_name in column_names:
-qry = qry.where(Column(col_name) == value)
-return qry
+query = query.where(Column(col_name) == value)
+return query
@classmethod
-def _latest_partition_from_df(cls, df):
+def _latest_partition_from_df(cls, df) -> Optional[Tuple[str, ...]]:
if not df.empty:
return df.to_records(index=False)[0].item()
+return None
@classmethod
-def latest_partition(cls, table_name, schema, database, show_first=False):
+def latest_partition(
+cls, table_name: str, schema: str, database, show_first: bool = False
+) -> Tuple[List[str], Optional[Tuple[str, ...]]]:
"""Returns col name and the latest (max) partition value for a table
:param table_name: the name of the table
-:type table_name: str
:param schema: schema / database / namespace
-:type schema: str
:param database: database query will be run against
:type database: models.Database
:param show_first: displays the value for the first partitioning key
if there are many partitioning keys
:type show_first: bool
>>> latest_partition('foo_table')
-('ds', '2018-01-01')
+(['ds'], ('2018-01-01',))
Review comment:
Yeah, i'm not certain if this is right either... There's been quite a few
changes to the presto db engine spec lately which have introduced bugs. Once
the circular imports are sorted out it'd probably be worthwhile to come back to
this; I'd be happy to pair code/work with you on this
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request #8058: Add docstrings and typing to db_engine_specs and sql_parse
etr2460 commented on a change in pull request #8058: Add docstrings and typing
to db_engine_specs and sql_parse
URL:
https://github.com/apache/incubator-superset/pull/8058#discussion_r315417347
##
File path: superset/db_engine_specs/presto.py
##
@@ -944,22 +955,22 @@ def where_latest_partition(cls, table_name, schema,
database, qry, columns=None)
column_names = {column.get("name") for column in columns or []}
for col_name, value in zip(col_names, values):
if col_name in column_names:
-qry = qry.where(Column(col_name) == value)
-return qry
+query = query.where(Column(col_name) == value)
+return query
@classmethod
def _latest_partition_from_df(cls, df):
if not df.empty:
return df.to_records(index=False)[0].item()
@classmethod
-def latest_partition(cls, table_name, schema, database, show_first=False):
+def latest_partition(
+cls, table_name: str, schema: str, database, show_first: bool = False
+) -> Tuple[str, str]:
Review comment:
This typing isn't quite right here:
I believe we return a `Tuple[List[str], Optional[List[str]]]`
`column_names` is a list (as we call `len` on it) and the second piece of
the tuple can be `None` (see method above)
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
