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 2022/05/13 16:55:45 UTC

[GitHub] [superset] hughhhh opened a new pull request, #20059: Save-col-on-query

hughhhh opened a new pull request, #20059:
URL: https://github.com/apache/superset/pull/20059

   <!---
   Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
   Example:
   fix(dashboard): load charts correctly
   -->
   
   ### SUMMARY
   <!--- Describe the change below, including rationale and design decisions -->
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   ### TESTING INSTRUCTIONS
   <!--- Required! What steps can be taken to manually verify the changes? -->
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] hughhhh commented on a diff in pull request #20059: feat: Save column data into json_metadata for all Query executions

Posted by GitBox <gi...@apache.org>.
hughhhh commented on code in PR #20059:
URL: https://github.com/apache/superset/pull/20059#discussion_r875180281


##########
superset/queries/dao.py:
##########
@@ -48,3 +50,20 @@ def update_saved_query_exec_info(query_id: int) -> None:
                 saved_query.rows = query.rows
                 saved_query.last_run = datetime.now()
             db.session.commit()
+
+    @staticmethod
+    def save_metadata(query: Query, query_payload: str) -> None:
+        # parse payload
+        try:
+            payload: Dict[str, Any] = json.loads(query_payload)

Review Comment:
   @ktmud so looking at the code we'd have to do a `json.dump` before returning the payload for it to fit into the `json_success` function which needs a `str`
   
   https://github.com/hve-labs/superset/blob/060b5c0e177b89eff9b6ace83b7df235f25cc198/superset/views/base.py#L159
   



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] ktmud commented on pull request #20059: feat: Save column data into json_metadata for all Query executions

Posted by GitBox <gi...@apache.org>.
ktmud commented on PR #20059:
URL: https://github.com/apache/superset/pull/20059#issuecomment-1127987133

   Is there a reason not to reuse the ExtraJSONMixin？


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] ktmud commented on a diff in pull request #20059: feat: Save column data into json_metadata for all Query executions

Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #20059:
URL: https://github.com/apache/superset/pull/20059#discussion_r875187421


##########
superset/queries/dao.py:
##########
@@ -48,3 +50,20 @@ def update_saved_query_exec_info(query_id: int) -> None:
                 saved_query.rows = query.rows
                 saved_query.last_run = datetime.now()
             db.session.commit()
+
+    @staticmethod
+    def save_metadata(query: Query, query_payload: str) -> None:
+        # parse payload
+        try:
+            payload: Dict[str, Any] = json.loads(query_payload)

Review Comment:
   Yeah, but can we maybe update [`ExecutionContextConvertor.to_payload`](https://github.com/apache/superset/commit/0d0c759cfed3dc72aaca029b5da1db1acfa393a9) to return Python dict and only do JSON dumps in [`ExecuteSqlCommand.run`](https://github.com/apache/superset/blob/8e29ec5a6685867ffc035d20999c54c2abe36fb1/superset/sqllab/command.py#L99-L101)? Looks like `to_payload` is only used in this one place.
   
   Or better yet, replace [json_success](https://github.com/apache/superset/blob/060b5c0e177b89eff9b6ace83b7df235f25cc198/superset/views/core.py#L2447) in `_create_response_from_execution_context` with `self.json_response(...)`, which dumps dict to strings automatically.
   
   We should do JSON serialization only when we are very close to sending it to the response.
   
   cc @ofekisr 



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] ktmud commented on a diff in pull request #20059: feat: Save column data into json_metadata for all Query executions

Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #20059:
URL: https://github.com/apache/superset/pull/20059#discussion_r874208080


##########
superset/queries/dao.py:
##########
@@ -48,3 +50,20 @@ def update_saved_query_exec_info(query_id: int) -> None:
                 saved_query.rows = query.rows
                 saved_query.last_run = datetime.now()
             db.session.commit()
+
+    @staticmethod
+    def save_metadata(query: Query, query_payload: str) -> None:
+        # parse payload
+        try:
+            payload: Dict[str, Any] = json.loads(query_payload)

Review Comment:
   The fact that we are dumping a json then reloading it within the same process doesn't bod well for me. It's probably also not a great idea to passing strings around between functions.
   
   Can we refactor `_execution_context_convertor` to return the raw dict instead? (Maybe add a new public function?)



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] ktmud commented on a diff in pull request #20059: feat: Save column data into json_metadata for all Query executions

Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #20059:
URL: https://github.com/apache/superset/pull/20059#discussion_r875394272


##########
superset/sqllab/command.py:
##########
@@ -94,11 +96,19 @@ def run(  # pylint: disable=too-many-statements,useless-suppression
                 status = SqlJsonExecutionStatus.QUERY_ALREADY_CREATED
             else:
                 status = self._run_sql_json_exec_from_scratch()
+
+            self._execution_context_convertor.set_payload(
+                self._execution_context, status
+            )
+
+            # save columns into metadata_json
+            self._query_dao.save_metadata(
+                self._execution_context.query, self._execution_context_convertor.payload
+            )
+
             return {
                 "status": status,
-                "payload": self._execution_context_convertor.to_payload(
-                    self._execution_context, status
-                ),
+                "payload": self._execution_context_convertor.serialize_payload(),

Review Comment:
   Can we maybe just let the command return raw dict here, too, and use `self.json_response` in whatever view handler the result may be used, so that json.dumps in the executor itself can be cleaned up?



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] hughhhh commented on pull request #20059: feat: Save column data into json_metadata for all Query executions

Posted by GitBox <gi...@apache.org>.
hughhhh commented on PR #20059:
URL: https://github.com/apache/superset/pull/20059#issuecomment-1128063638

   > Is there a reason not to reuse the ExtraJSONMixin？
   
   I'm using the extra mixin here
   
   https://github.com/apache/superset/pull/20059/files#diff-68d044828e32dbc106f7cece56d7aeeccbe2eb455cf33307df4fc9d07dfb1fd5R67


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] codecov[bot] commented on pull request #20059: feat: Save column data into json_metadata for all Query executions

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #20059:
URL: https://github.com/apache/superset/pull/20059#issuecomment-1128112214

   # [Codecov](https://codecov.io/gh/apache/superset/pull/20059?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#20059](https://codecov.io/gh/apache/superset/pull/20059?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3618c8a) into [master](https://codecov.io/gh/apache/superset/commit/ddc01ea7813ef7c02cfc2aee7cbf554a45628f25?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ddc01ea) will **decrease** coverage by `12.07%`.
   > The diff coverage is `78.94%`.
   
   ```diff
   @@             Coverage Diff             @@
   ##           master   #20059       +/-   ##
   ===========================================
   - Coverage   66.45%   54.38%   -12.08%     
   ===========================================
     Files        1721     1721               
     Lines       64479    64496       +17     
     Branches     6795     6795               
   ===========================================
   - Hits        42852    35075     -7777     
   - Misses      19897    27691     +7794     
     Partials     1730     1730               
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | hive | `?` | |
   | mysql | `?` | |
   | postgres | `?` | |
   | presto | `53.56% <78.94%> (+0.01%)` | :arrow_up: |
   | python | `57.61% <78.94%> (-25.01%)` | :arrow_down: |
   | sqlite | `?` | |
   | unit | `49.40% <63.15%> (+0.01%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/20059?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [superset/sqllab/command.py](https://codecov.io/gh/apache/superset/pull/20059/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsbGFiL2NvbW1hbmQucHk=) | `84.87% <75.00%> (-0.35%)` | :arrow_down: |
   | [superset/queries/dao.py](https://codecov.io/gh/apache/superset/pull/20059/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcXVlcmllcy9kYW8ucHk=) | `90.62% <76.92%> (-9.38%)` | :arrow_down: |
   | [superset/sql\_lab.py](https://codecov.io/gh/apache/superset/pull/20059/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX2xhYi5weQ==) | `79.15% <100.00%> (-2.71%)` | :arrow_down: |
   | [superset/views/core.py](https://codecov.io/gh/apache/superset/pull/20059/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `35.14% <100.00%> (-42.25%)` | :arrow_down: |
   | [superset/utils/dashboard\_import\_export.py](https://codecov.io/gh/apache/superset/pull/20059/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvZGFzaGJvYXJkX2ltcG9ydF9leHBvcnQucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset/key\_value/commands/upsert.py](https://codecov.io/gh/apache/superset/pull/20059/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3Vwc2VydC5weQ==) | `0.00% <0.00%> (-89.59%)` | :arrow_down: |
   | [superset/key\_value/commands/update.py](https://codecov.io/gh/apache/superset/pull/20059/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `0.00% <0.00%> (-89.37%)` | :arrow_down: |
   | [superset/key\_value/commands/delete.py](https://codecov.io/gh/apache/superset/pull/20059/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZS5weQ==) | `0.00% <0.00%> (-85.30%)` | :arrow_down: |
   | [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/20059/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.19%)` | :arrow_down: |
   | [superset/key\_value/commands/delete\_expired.py](https://codecov.io/gh/apache/superset/pull/20059/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZV9leHBpcmVkLnB5) | `0.00% <0.00%> (-80.77%)` | :arrow_down: |
   | ... and [273 more](https://codecov.io/gh/apache/superset/pull/20059/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/20059?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/20059?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ddc01ea...3618c8a](https://codecov.io/gh/apache/superset/pull/20059?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] ktmud commented on a diff in pull request #20059: feat: Save column data into json_metadata for all Query executions

Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #20059:
URL: https://github.com/apache/superset/pull/20059#discussion_r875187421


##########
superset/queries/dao.py:
##########
@@ -48,3 +50,20 @@ def update_saved_query_exec_info(query_id: int) -> None:
                 saved_query.rows = query.rows
                 saved_query.last_run = datetime.now()
             db.session.commit()
+
+    @staticmethod
+    def save_metadata(query: Query, query_payload: str) -> None:
+        # parse payload
+        try:
+            payload: Dict[str, Any] = json.loads(query_payload)

Review Comment:
   Yeah, but can we maybe update [`ExecutionContextConvertor.to_payload`](https://github.com/apache/superset/commit/0d0c759cfed3dc72aaca029b5da1db1acfa393a9) to return Python dict and only do JSON dumps in the [`ExecuteSqlCommand.run`](https://github.com/apache/superset/blob/8e29ec5a6685867ffc035d20999c54c2abe36fb1/superset/sqllab/command.py#L99-L101)? Looks like `to_payload` is only used in this one place.



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] ktmud commented on a diff in pull request #20059: feat: Save column data into json_metadata for all Query executions

Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #20059:
URL: https://github.com/apache/superset/pull/20059#discussion_r876050626


##########
superset/sqllab/command.py:
##########
@@ -94,11 +96,19 @@ def run(  # pylint: disable=too-many-statements,useless-suppression
                 status = SqlJsonExecutionStatus.QUERY_ALREADY_CREATED
             else:
                 status = self._run_sql_json_exec_from_scratch()
+
+            self._execution_context_convertor.set_payload(
+                self._execution_context, status
+            )
+
+            # save columns into metadata_json
+            self._query_dao.save_metadata(
+                self._execution_context.query, self._execution_context_convertor.payload
+            )
+
             return {
                 "status": status,
-                "payload": self._execution_context_convertor.to_payload(
-                    self._execution_context, status
-                ),
+                "payload": self._execution_context_convertor.serialize_payload(),

Review Comment:
   Sounds good.



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] ktmud commented on a diff in pull request #20059: feat: Save column data into json_metadata for all Query executions

Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #20059:
URL: https://github.com/apache/superset/pull/20059#discussion_r875187421


##########
superset/queries/dao.py:
##########
@@ -48,3 +50,20 @@ def update_saved_query_exec_info(query_id: int) -> None:
                 saved_query.rows = query.rows
                 saved_query.last_run = datetime.now()
             db.session.commit()
+
+    @staticmethod
+    def save_metadata(query: Query, query_payload: str) -> None:
+        # parse payload
+        try:
+            payload: Dict[str, Any] = json.loads(query_payload)

Review Comment:
   Yeah, but can we maybe update [`ExecutionContextConvertor.to_payload`](https://github.com/apache/superset/commit/0d0c759cfed3dc72aaca029b5da1db1acfa393a9) to return Python dict and only do JSON dumps in [`ExecuteSqlCommand.run`](https://github.com/apache/superset/blob/8e29ec5a6685867ffc035d20999c54c2abe36fb1/superset/sqllab/command.py#L99-L101)? Looks like `to_payload` is only used in this one place.



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] hughhhh merged pull request #20059: feat: Save column data into json_metadata for all Query executions

Posted by GitBox <gi...@apache.org>.
hughhhh merged PR #20059:
URL: https://github.com/apache/superset/pull/20059


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] AAfghahi commented on a diff in pull request #20059: feat: Save column data into json_metadata for all Query executions

Posted by GitBox <gi...@apache.org>.
AAfghahi commented on code in PR #20059:
URL: https://github.com/apache/superset/pull/20059#discussion_r874182780


##########
superset/queries/dao.py:
##########
@@ -48,3 +50,20 @@ def update_saved_query_exec_info(query_id: int) -> None:
                 saved_query.rows = query.rows
                 saved_query.last_run = datetime.now()
             db.session.commit()
+
+    @staticmethod
+    def save_metadata(query: Query, query_payload: str) -> None:
+        # parse payload
+        try:
+            payload: Dict[str, Any] = json.loads(query_payload)
+        except json.JSONDecodeError:
+            logger.warning("Error parsing query_payload: %s", query_payload)
+            return None
+
+        # pull relevant data from payload and store in json_metadata
+        columns = payload.get("columns", {})
+
+        # save payload into query object
+        db.session.add(query)
+        query.set_extra_json_key("columns", columns)
+        return None

Review Comment:
   why do these have return statement? 



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] ktmud commented on pull request #20059: feat: Save column data into json_metadata for all Query executions

Posted by GitBox <gi...@apache.org>.
ktmud commented on PR #20059:
URL: https://github.com/apache/superset/pull/20059#issuecomment-1128200874

   My apologies. The fact that the the command is called `save_metadata` made me think the `ExtraJSONMixin` was not used. Not sure if we need to change it , but it's worth checking what other modules are doing for similar operations.


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] hughhhh commented on a diff in pull request #20059: feat: Save column data into json_metadata for all Query executions

Posted by GitBox <gi...@apache.org>.
hughhhh commented on code in PR #20059:
URL: https://github.com/apache/superset/pull/20059#discussion_r875180281


##########
superset/queries/dao.py:
##########
@@ -48,3 +50,20 @@ def update_saved_query_exec_info(query_id: int) -> None:
                 saved_query.rows = query.rows
                 saved_query.last_run = datetime.now()
             db.session.commit()
+
+    @staticmethod
+    def save_metadata(query: Query, query_payload: str) -> None:
+        # parse payload
+        try:
+            payload: Dict[str, Any] = json.loads(query_payload)

Review Comment:
   @ktmud so looking at the code we'd have to do a `json.dump` before returning the payload for it to fit into the `json_success` function which needs a `str`
   
   https://github.com/hve-labs/superset/blob/060b5c0e177b89eff9b6ace83b7df235f25cc198/superset/views/base.py#L159
   



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] hughhhh commented on a diff in pull request #20059: feat: Save column data into json_metadata for all Query executions

Posted by GitBox <gi...@apache.org>.
hughhhh commented on code in PR #20059:
URL: https://github.com/apache/superset/pull/20059#discussion_r876019278


##########
superset/sqllab/command.py:
##########
@@ -94,11 +96,19 @@ def run(  # pylint: disable=too-many-statements,useless-suppression
                 status = SqlJsonExecutionStatus.QUERY_ALREADY_CREATED
             else:
                 status = self._run_sql_json_exec_from_scratch()
+
+            self._execution_context_convertor.set_payload(
+                self._execution_context, status
+            )
+
+            # save columns into metadata_json
+            self._query_dao.save_metadata(
+                self._execution_context.query, self._execution_context_convertor.payload
+            )
+
             return {
                 "status": status,
-                "payload": self._execution_context_convertor.to_payload(
-                    self._execution_context, status
-                ),
+                "payload": self._execution_context_convertor.serialize_payload(),

Review Comment:
   but there is specific logic in the `json.dumps` that needs the `status` to understand how to serialize. I don't see a clean way to do this unless we move all this logic outside of the command.
   
   Can we revisit this in another refactor ticket? 



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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