[sqllab] fix: return pandas records in execute_sql_statements

[nytai]

CATEGORY

Choose one

• Bug Fix
• Enhancement (new features, refinement)
• Refactor
• Add tests
• Build / Development Environment
• Documentation

SUMMARY

SQLLAB_BACKEND_PERSISTENCE feature stores the results in the results backend which is used to rehydrate tab states. Data is incorrectly being returned in serialized binary form due to the assumption that if we're storing results the query is being executed asynchronously. This pr fixes that assumption by also checking if we're looking to return results (in which case we also use the df_to_records and JSON serialization approach).

This is obviously not the most performant way of handling this as were doing pyarrow + msgpack to store the results in the results backend, as well as pandas + json to return the results, however it's the easiest way to get SQLLAB_BACKEND_PERSISTENCE + RESULTS_BACKEND_USE_MSGPACK working together without a significant rewrite/refactor.

TEST PLAN

• tested locally that sqllab works with SQLLAB_BACKEND_PERSISTENCE feature enabled
• tested locally that sqllab works with SQLLAB_BACKEND_PERSISTENCE feature disabled

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

@robdiciuccio @betodealmeida

[robdiciuccio]

Ideally, we'll always want to serialize to msgpack (vs json) when persisting to the results backend due to the performance gains. Instead of skipping msgpack serialization, can the logic used to fetch stored results and rehydrate the tab state perform the proper deserialization?

I'm still not clear where the issue lies with synchronous queries returning bad data. Is msgpack modifying the payload dict passed into _serialize_payload?

Previously, RESULTS_BACKEND was only used for async queries. What is the expected behavior of server-persisted tab state if no results backend is configured? Should we update the documentation to explain this new use case?

[codecov-io]

Codecov Report

Merging #9102 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #9102   +/-   ##
======================================
  Coverage    59.1%   59.1%           
======================================
  Files         372     372           
  Lines       11922   11922           
  Branches     2919    2919           
======================================
  Hits         7046    7046           
  Misses       4694    4694           
  Partials      182     182

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a005e45...460b46d. Read the comment docs.

[nytai]

@robdiciuccio Looks like it's the pyarrow data that's getting sent to the frontend, thus it ends up as '[bytes]' for that key. As for if the feature flag is on and no results backend is configured the following is displayed to the user: