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/01/28 16:12:08 UTC
[GitHub] [incubator-superset] villebro opened a new pull request #9033: Add
schema to function retrieval and implement for Snowflake
villebro opened a new pull request #9033: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033
### CATEGORY
Choose one
- [ ] Bug Fix
- [x] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
Some databases, like Snowflake, support defining UDFs per schema. This PR adds the possibility to retrieve both global functions and schema-specific UDFs, and implements said functionality for Snowflake. Achieving this required moving the function retrieval from the database api to the table api, and making small modifications in the frontend code.
### SCREENSHOTS
In the first picture we are browsing a schema where the UDF `SFWHO` is available:
<img width="697" alt="Screenshot 2020-01-28 at 17 51 54" src="https://user-images.githubusercontent.com/33317356/73281746-1567ae80-41f9-11ea-9d0c-07a7d7135d12.png">
When browsing a schema without UDFs, only the global functions show up:
<img width="698" alt="Screenshot 2020-01-28 at 17 51 16" src="https://user-images.githubusercontent.com/33317356/73281661-fc5efd80-41f8-11ea-80a7-13a342060e7c.png">
### TEST PLAN
Tested locally.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] 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
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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 issue #9033: feat: Add
schema to function retrieval and implement for Snowflake
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033#issuecomment-584134159
After reviewing this after a short break it feels the added complexity is difficult to justify given the very limited utility of schema specific UDFs. Closing this for now, will revive this at a later date if the need resurfaces.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on issue #9033: feat: Add
schema to function retrieval and implement for Snowflake
Posted by GitBox <gi...@apache.org>.
etr2460 commented on issue #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033#issuecomment-580864831
Seems fine to me, i'll approve!
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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
#9033: feat: Add schema to function retrieval and implement for Snowflake
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033#discussion_r373599945
##########
File path: superset/assets/src/SqlLab/actions/sqlLab.js
##########
@@ -785,8 +786,17 @@ export function queryEditorSetSchemaOptions(queryEditor, options) {
return { type: QUERY_EDITOR_SET_SCHEMA_OPTIONS, queryEditor, options };
}
-export function queryEditorSetTableOptions(queryEditor, options) {
- return { type: QUERY_EDITOR_SET_TABLE_OPTIONS, queryEditor, options };
+export function queryEditorSetTableOptions(
Review comment:
True, will adjust accordingly.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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
#9033: feat: Add schema to function retrieval and implement for Snowflake
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033#discussion_r372780260
##########
File path: superset/db_engine_specs/snowflake.py
##########
@@ -74,3 +84,32 @@ def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]:
if tt == "TIMESTAMP":
return f"""TO_TIMESTAMP('{dttm.isoformat(timespec="microseconds")}')"""
return None
+
+ @classmethod
+ @cache.memoize()
+ def get_function_names(
+ cls, database: "Database", schema: Optional[str]
+ ) -> List[str]:
+ """
+ Get a list of function names that are able to be called on the database.
+ Used for SQL Lab autocomplete. Returns global functions, excluding system
+ functions (starting with "SYSTEM$") and UDFs that are available in the chosen
+ schema.
+
+ :param database: The database to get functions for
+ :param schema: The schema to get functions for
+ :return: A list of function names useable in the database
+ """
+
+ def is_valid_function(func_row: pd.Series) -> bool:
+ func_schema, func_name = func_row["schema_name"], func_row["name"]
+ if func_regex.match(func_name) and (
+ func_schema in (None, "")
+ or schema is None
Review comment:
Also fixed.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9033: feat: Add schema to function retrieval and implement for
Snowflake
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033#discussion_r372661726
##########
File path: superset/db_engine_specs/snowflake.py
##########
@@ -74,3 +84,32 @@ def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]:
if tt == "TIMESTAMP":
return f"""TO_TIMESTAMP('{dttm.isoformat(timespec="microseconds")}')"""
return None
+
+ @classmethod
+ @cache.memoize()
+ def get_function_names(
+ cls, database: "Database", schema: Optional[str]
+ ) -> List[str]:
+ """
+ Get a list of function names that are able to be called on the database.
+ Used for SQL Lab autocomplete. Returns global functions, excluding system
+ functions (starting with "SYSTEM$") and UDFs that are available in the chosen
+ schema.
+
+ :param database: The database to get functions for
+ :param schema: The schema to get functions for
+ :return: A list of function names useable in the database
+ """
+
+ def is_valid_function(func_row: pd.Series) -> bool:
+ func_schema, func_name = func_row["schema_name"], func_row["name"]
+ if func_regex.match(func_name) and (
+ func_schema in (None, "")
Review comment:
Can this not be a falsy check, i.e. if `not func_schema`?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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
#9033: feat: Add schema to function retrieval and implement for Snowflake
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033#discussion_r372780211
##########
File path: superset/db_engine_specs/snowflake.py
##########
@@ -74,3 +84,32 @@ def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]:
if tt == "TIMESTAMP":
return f"""TO_TIMESTAMP('{dttm.isoformat(timespec="microseconds")}')"""
return None
+
+ @classmethod
+ @cache.memoize()
+ def get_function_names(
+ cls, database: "Database", schema: Optional[str]
+ ) -> List[str]:
+ """
+ Get a list of function names that are able to be called on the database.
+ Used for SQL Lab autocomplete. Returns global functions, excluding system
+ functions (starting with "SYSTEM$") and UDFs that are available in the chosen
+ schema.
+
+ :param database: The database to get functions for
+ :param schema: The schema to get functions for
+ :return: A list of function names useable in the database
+ """
+
+ def is_valid_function(func_row: pd.Series) -> bool:
Review comment:
Fixed.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io commented on issue #9033: feat: Add
schema to function retrieval and implement for Snowflake
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033#issuecomment-579374243
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9033?src=pr&el=h1) Report
> Merging [#9033](https://codecov.io/gh/apache/incubator-superset/pull/9033?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/dc60db2a433e7ac04c40f8e9fe43617717ebb7d8?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `57.14%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9033?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9033 +/- ##
==========================================
- Coverage 59.43% 59.43% -0.01%
==========================================
Files 369 369
Lines 11743 11742 -1
Branches 2884 2883 -1
==========================================
- Hits 6980 6979 -1
Misses 4584 4584
Partials 179 179
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9033?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/assets/src/SqlLab/reducers/sqlLab.js](https://codecov.io/gh/apache/incubator-superset/pull/9033/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvcmVkdWNlcnMvc3FsTGFiLmpz) | `35.39% <ø> (ø)` | :arrow_up: |
| [...uperset/assets/src/SqlLab/components/SqlEditor.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9033/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvY29tcG9uZW50cy9TcWxFZGl0b3IuanN4) | `51.63% <ø> (-0.32%)` | :arrow_down: |
| [superset/assets/src/SqlLab/actions/sqlLab.js](https://codecov.io/gh/apache/incubator-superset/pull/9033/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvYWN0aW9ucy9zcWxMYWIuanM=) | `61.96% <0%> (ø)` | :arrow_up: |
| [.../assets/src/SqlLab/components/SqlEditorLeftBar.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9033/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvY29tcG9uZW50cy9TcWxFZGl0b3JMZWZ0QmFyLmpzeA==) | `40% <0%> (ø)` | :arrow_up: |
| [superset/assets/src/components/TableSelector.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9033/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9jb21wb25lbnRzL1RhYmxlU2VsZWN0b3IuanN4) | `78.03% <100%> (ø)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9033?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9033?src=pr&el=footer). Last update [dc60db2...da22997](https://codecov.io/gh/apache/incubator-superset/pull/9033?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9033: feat: Add schema to function retrieval and implement for
Snowflake
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033#discussion_r372661320
##########
File path: superset/db_engine_specs/snowflake.py
##########
@@ -14,12 +14,22 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+import re
from datetime import datetime
-from typing import Optional
+from typing import List, Optional, TYPE_CHECKING
from urllib import parse
+import pandas as pd
+
+from superset import cache
from superset.db_engine_specs.postgres import PostgresBaseEngineSpec
+if TYPE_CHECKING:
+ # prevent circular imports
+ from superset.models.core import Database # pylint: disable=unused-import
+
+func_regex = re.compile(r"^((?<!SYSTEM$)(\w))+$")
Review comment:
Nit. `func_regex` -> `FUNC_REGEX` as it's globally defined. Note if configured correctly `pylint` should error 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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
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
#9033: feat: Add schema to function retrieval and implement for Snowflake
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033#discussion_r372778140
##########
File path: superset/db_engine_specs/hive.py
##########
@@ -429,12 +429,26 @@ def execute( # type: ignore
@classmethod
@cache.memoize()
- def get_function_names(cls, database: "Database") -> List[str]:
+ def _get_function_names(cls, database: "Database") -> List[str]:
Review comment:
Very good point. Somehow I instinctively wanted to avoid the overhead of regenerating the nested function, but seems I grossly overestimated the cost: "[...]it takes less time than creating an empty dict" https://stackoverflow.com/questions/7839632/is-there-an-overhead-when-nesting-functions-in-python
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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
#9033: feat: Add schema to function retrieval and implement for Snowflake
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033#discussion_r372778943
##########
File path: superset/db_engine_specs/hive.py
##########
@@ -429,12 +429,26 @@ def execute( # type: ignore
@classmethod
@cache.memoize()
Review comment:
Thanks for pointing this out, I completely agree with this assessment.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9033: feat: Add schema to function retrieval and implement for
Snowflake
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033#discussion_r372657712
##########
File path: superset/db_engine_specs/hive.py
##########
@@ -429,12 +429,26 @@ def execute( # type: ignore
@classmethod
@cache.memoize()
- def get_function_names(cls, database: "Database") -> List[str]:
+ def _get_function_names(cls, database: "Database") -> List[str]:
Review comment:
@villebro the `_get_function_names` function could be nested within the `get_function_names` method if you like.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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
#9033: feat: Add schema to function retrieval and implement for Snowflake
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033#discussion_r372779905
##########
File path: superset/db_engine_specs/snowflake.py
##########
@@ -14,12 +14,22 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+import re
from datetime import datetime
-from typing import Optional
+from typing import List, Optional, TYPE_CHECKING
from urllib import parse
+import pandas as pd
+
+from superset import cache
from superset.db_engine_specs.postgres import PostgresBaseEngineSpec
+if TYPE_CHECKING:
+ # prevent circular imports
+ from superset.models.core import Database # pylint: disable=unused-import
+
+func_regex = re.compile(r"^((?<!SYSTEM$)(\w))+$")
Review comment:
Fixed.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9033: feat: Add schema to function retrieval and implement for
Snowflake
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033#discussion_r372664042
##########
File path: superset/db_engine_specs/snowflake.py
##########
@@ -74,3 +84,32 @@ def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]:
if tt == "TIMESTAMP":
return f"""TO_TIMESTAMP('{dttm.isoformat(timespec="microseconds")}')"""
return None
+
+ @classmethod
+ @cache.memoize()
+ def get_function_names(
+ cls, database: "Database", schema: Optional[str]
+ ) -> List[str]:
+ """
+ Get a list of function names that are able to be called on the database.
+ Used for SQL Lab autocomplete. Returns global functions, excluding system
+ functions (starting with "SYSTEM$") and UDFs that are available in the chosen
+ schema.
+
+ :param database: The database to get functions for
+ :param schema: The schema to get functions for
+ :return: A list of function names useable in the database
+ """
+
+ def is_valid_function(func_row: pd.Series) -> bool:
+ func_schema, func_name = func_row["schema_name"], func_row["name"]
+ if func_regex.match(func_name) and (
+ func_schema in (None, "")
+ or schema is None
Review comment:
Similarly could this be `not schema`?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9033: feat: Add schema to function retrieval and implement for
Snowflake
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033#discussion_r372659361
##########
File path: superset/db_engine_specs/hive.py
##########
@@ -429,12 +429,26 @@ def execute( # type: ignore
@classmethod
@cache.memoize()
Review comment:
Somewhat unrelated to your change I noticed there a relative new caching structure defined [here](https://github.com/apache/incubator-superset/blob/master/superset/utils/cache.py#L27) which provides caching at the database level. I'm not sure if there's merit in using that in favor of the global cache, but I do wonder if we should be using the `tables_cache` rather than the `cache` for table, i.e., SQL Lab, related caching.
Note it's all a little confusing when a cached object expires given the plethora of caches which now seem to exist.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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
#9033: feat: Add schema to function retrieval and implement for Snowflake
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033#discussion_r372780177
##########
File path: superset/db_engine_specs/snowflake.py
##########
@@ -74,3 +84,32 @@ def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]:
if tt == "TIMESTAMP":
return f"""TO_TIMESTAMP('{dttm.isoformat(timespec="microseconds")}')"""
return None
+
+ @classmethod
+ @cache.memoize()
+ def get_function_names(
+ cls, database: "Database", schema: Optional[str]
+ ) -> List[str]:
+ """
+ Get a list of function names that are able to be called on the database.
+ Used for SQL Lab autocomplete. Returns global functions, excluding system
+ functions (starting with "SYSTEM$") and UDFs that are available in the chosen
+ schema.
+
+ :param database: The database to get functions for
+ :param schema: The schema to get functions for
+ :return: A list of function names useable in the database
+ """
+
+ def is_valid_function(func_row: pd.Series) -> bool:
+ func_schema, func_name = func_row["schema_name"], func_row["name"]
+ if func_regex.match(func_name) and (
+ func_schema in (None, "")
Review comment:
You're right, not sure what I was thinking here..
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on issue #9033: feat:
Add schema to function retrieval and implement for Snowflake
Posted by GitBox <gi...@apache.org>.
john-bodley commented on issue #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033#issuecomment-579982417
cc @etr2460
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro closed pull request #9033: feat: Add
schema to function retrieval and implement for Snowflake
Posted by GitBox <gi...@apache.org>.
villebro closed pull request #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9033: feat: Add schema to function retrieval and implement for
Snowflake
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033#discussion_r372662013
##########
File path: superset/db_engine_specs/snowflake.py
##########
@@ -74,3 +84,32 @@ def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]:
if tt == "TIMESTAMP":
return f"""TO_TIMESTAMP('{dttm.isoformat(timespec="microseconds")}')"""
return None
+
+ @classmethod
+ @cache.memoize()
+ def get_function_names(
+ cls, database: "Database", schema: Optional[str]
+ ) -> List[str]:
+ """
+ Get a list of function names that are able to be called on the database.
+ Used for SQL Lab autocomplete. Returns global functions, excluding system
+ functions (starting with "SYSTEM$") and UDFs that are available in the chosen
+ schema.
+
+ :param database: The database to get functions for
+ :param schema: The schema to get functions for
+ :return: A list of function names useable in the database
+ """
+
+ def is_valid_function(func_row: pd.Series) -> bool:
Review comment:
Nit. `is_valid_function` -> `_is_valid_function` since it's nested.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9033: feat: Add schema to function retrieval and implement for Snowflake
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033#discussion_r373595873
##########
File path: superset/assets/src/SqlLab/actions/sqlLab.js
##########
@@ -785,8 +786,17 @@ export function queryEditorSetSchemaOptions(queryEditor, options) {
return { type: QUERY_EDITOR_SET_SCHEMA_OPTIONS, queryEditor, options };
}
-export function queryEditorSetTableOptions(queryEditor, options) {
- return { type: QUERY_EDITOR_SET_TABLE_OPTIONS, queryEditor, options };
+export function queryEditorSetTableOptions(
Review comment:
in general, i think it's best practice to make the function name match the action type. let's correct that?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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 issue #9033: feat: Add
schema to function retrieval and implement for Snowflake
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033#issuecomment-580837645
> This new change means that you won't get function autocomplete until you select a schema in SQL Lab, correct? Do we think that's ok behavior for all dbs, even if it's not required for Presto/Hive?
If "Multi Schema Metadata Fetch" is enabled, functions will be fetched (along with table names). Do you think this is ok?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #9033:
feat: Add schema to function retrieval and implement for Snowflake
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033#issuecomment-579374243
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9033?src=pr&el=h1) Report
> Merging [#9033](https://codecov.io/gh/apache/incubator-superset/pull/9033?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/6416ef51fffea9dc13ee2a89ac8fb3b72bb14239?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `55.55%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9033?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9033 +/- ##
==========================================
- Coverage 59.45% 59.45% -0.01%
==========================================
Files 369 369
Lines 11747 11746 -1
Branches 2888 2887 -1
==========================================
- Hits 6984 6983 -1
Misses 4584 4584
Partials 179 179
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9033?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...uperset/assets/src/SqlLab/components/SqlEditor.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9033/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvY29tcG9uZW50cy9TcWxFZGl0b3IuanN4) | `51.63% <ø> (-0.32%)` | :arrow_down: |
| [superset/assets/src/SqlLab/reducers/sqlLab.js](https://codecov.io/gh/apache/incubator-superset/pull/9033/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvcmVkdWNlcnMvc3FsTGFiLmpz) | `35.39% <0%> (ø)` | :arrow_up: |
| [.../assets/src/SqlLab/components/SqlEditorLeftBar.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9033/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvY29tcG9uZW50cy9TcWxFZGl0b3JMZWZ0QmFyLmpzeA==) | `40% <0%> (ø)` | :arrow_up: |
| [superset/assets/src/components/TableSelector.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9033/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9jb21wb25lbnRzL1RhYmxlU2VsZWN0b3IuanN4) | `78.35% <100%> (ø)` | :arrow_up: |
| [superset/assets/src/SqlLab/actions/sqlLab.js](https://codecov.io/gh/apache/incubator-superset/pull/9033/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvYWN0aW9ucy9zcWxMYWIuanM=) | `61.96% <33.33%> (ø)` | :arrow_up: |
| [.../assets/src/SqlLab/components/AceEditorWrapper.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9033/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvY29tcG9uZW50cy9BY2VFZGl0b3JXcmFwcGVyLmpzeA==) | `54.21% <0%> (ø)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9033?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9033?src=pr&el=footer). Last update [6416ef5...652c6ea](https://codecov.io/gh/apache/incubator-superset/pull/9033?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9033: feat: Add schema to function retrieval and implement for Snowflake
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033#discussion_r373597481
##########
File path: superset/db_engine_specs/presto.py
##########
@@ -947,13 +948,20 @@ def latest_sub_partition(cls, table_name, schema, database, **kwargs):
return df.to_dict()[field_to_return][0]
@classmethod
- @cache.memoize()
- def get_function_names(cls, database: "Database") -> List[str]:
+ def get_function_names(
+ cls, database: "Database", schema: Optional[str]
+ ) -> List[str]:
"""
Get a list of function names that are able to be called on the database.
Used for SQL Lab autocomplete.
:param database: The database to get functions for
+ :param schema: The schema to get functions for (N/A for Presto)
Review comment:
Won't a schema still be passed for Presto even if it isn't being used? That would mean we'd still cache at the db + schema level for Presto (and hive) which isn't necessary.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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
#9033: feat: Add schema to function retrieval and implement for Snowflake
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033#discussion_r373600369
##########
File path: superset/db_engine_specs/presto.py
##########
@@ -947,13 +948,20 @@ def latest_sub_partition(cls, table_name, schema, database, **kwargs):
return df.to_dict()[field_to_return][0]
@classmethod
- @cache.memoize()
- def get_function_names(cls, database: "Database") -> List[str]:
+ def get_function_names(
+ cls, database: "Database", schema: Optional[str]
+ ) -> List[str]:
"""
Get a list of function names that are able to be called on the database.
Used for SQL Lab autocomplete.
:param database: The database to get functions for
+ :param schema: The schema to get functions for (N/A for Presto)
Review comment:
Memoization is only done for the nested function (see `_get_function_names` below), i.e. will not be cached per schema.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 edited a comment on issue #9033: feat:
Add schema to function retrieval and implement for Snowflake
Posted by GitBox <gi...@apache.org>.
etr2460 edited a comment on issue #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033#issuecomment-580864831
Seems fine to me, i'll approve once the JS changes are made
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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 issue #9033: feat: Add
schema to function retrieval and implement for Snowflake
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9033: feat: Add schema to function retrieval and implement for Snowflake
URL: https://github.com/apache/incubator-superset/pull/9033#issuecomment-580106297
Big thanks @john-bodley for the valuable review comments!
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org