Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix] Adding SIP-15 support for the query context #9219

Conversation

john-bodley
Copy link
Member

CATEGORY

Choose one

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

This PR ensures that the query context sets the query time range endpoints if undefined and SIP-15 is enabled.

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

Added unit test.

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

to: @etr2460 @villebro

@@ -973,7 +984,12 @@ def test_results_default_deserialization(self):
"sql": "SELECT * FROM birth_names LIMIT 100",
"status": utils.QueryStatus.PENDING,
}
serialized_data, selected_columns, all_columns, expanded_columns = sql_lab._serialize_and_expand_data(
(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is black formatting.

@@ -1016,7 +1032,12 @@ def test_results_msgpack_deserialization(self):
"sql": "SELECT * FROM birth_names LIMIT 100",
"status": utils.QueryStatus.PENDING,
}
serialized_data, selected_columns, all_columns, expanded_columns = sql_lab._serialize_and_expand_data(
(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is black formatting.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as quick fix, however I wonder if we should try to establish conventions for what should go into extras and what warrants a dedicated attribute, e.g. relative_start, relative_end etc, which IMO seem very much like extras. FYI @rusackas re: viz.py deprecation

@john-bodley
Copy link
Member Author

@villbro I agree. I’m not sure what the original intent was for extras and whether it was merely a convenient solution to circumvent the rigidity of the function signature. This is especially true for something like the query context which would also require changes to the superset-ui package.

@villebro
Copy link
Member

Looking at the current signature, I feel comfortable with most of the other parameters, but the relative_start and relative_end ones definitely belong in extras. You might have more context here, but I'm fairly we can move them over to extras without breaking anything. If not feel free to merge this as is, I can also address this later, as I'll probably be touching this code in the near future.

@john-bodley john-bodley merged commit 7b2c2e8 into apache:master Mar 2, 2020
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 0.36.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants