Add an extras field for Presto connector
#3962
Conversation
|
Just a quick observation about the json input - since we already have ACE editor bundled (as the query source editor) perhaps leverage it for syntax highlighting? |
|
@ranbena I'm not sure DynamicForm supports it and whether it worth the extra effort. |
It doesn't, though it's actually quite easy to change
As I tried this some time ago: It seems that the AceEditor changes its value, which conflicts with Antd Form. A way I've just found to handle that is to create a "useless class component" and use it instead (doesn't work with functional 😅). class AceTest extends React.Component {
render() {
return (<AceEditor {...this.props} />);
}
}After that would still need to align the validation mark. |
There was a problem hiding this comment.
Thank you for the contribution, @ktmud . It took me sometime to understand what feels wrong about this to me: having such an open ended option in the configuration means that we can never ensure backward compatibility (or know that we are going to break something). For this reason I would rather avoid this and just work on adding needed options one by one. While it's a bit more work, it ensures a more stable product in the long term.
I'm keeping this open to have a discussion, but as it is I think we won't merge it.
|
@arikfr - Adding options one by one is also not suitable option since over time lot of configurations have added or deprecated/removed in Presto, Also the configuration is version dependent. As of now there are close to ~100 feature flags in Presto & since now there are multiple Presto projects (PrestoDB vs PrestoSQL) some of the configurations are individual to the project. Internally we have done similar and so thereby easily able to upgrade presto versions with single change in config on Redash when needed. |
|
@arikfr Airflow and Superset both have something similar to most connection types: This is an advanced feature and will be used by advanced users only anyway. They should know what they are doing and how to upgrade. The only way I saw this causes backward capability issue was when the |
And load schemas one by one. This should improve performance for large Presto instances where a single schema may contain thousands of tables. Plus, in lower version of Presto, sometimes the old query will throw "outputFormat should not be accessed from a null StorageFormat" error (see prestodb/presto#6972). This change allows us to skip this error and still return valid results.
This makes it possible to allow arbitrary connection parameters that are not stored as configuration field. Also extended Dynamic Forms to allow JSON validation.
|
@ktmud , thanks for the PR! We've updated a lot of things now that we're Community-driven so - if you're still interested in getting this merged - would you mind rebasing off master to re-run the CI? We're trying to clean up our PR todo list, so if you're not interested, that's fine - we'll close the PR in about a week if we don't hear back. If you're interested in reopening the PR afterwards, we would also very much welcome that. |
What type of PR is this? (check all applicable)
Description
This adds a new "Extras" field for Presto connection, which is simply a JSON string and will be parsed as a dictionary and passed to PyHive's
presto.connectAPI.The most useful utilization of this feature is to allow self-assigned SSL certificate. For example, services like Qubole only allows SSL connection via self-assigned CA certificate. In which case we can then add
requests_kwargsin the extras:{"requests_kwargs": {"verify": "~/qubole.cert"} }We prefer this approach instead of the Qubole connector because Qubole charges you for every API call and the roundtrip from local EMR cluster to Qubole servers is not ideal.
This change also solves the need of adding extra fields for other connection arguments such as
Connection Sourceimplemented in #3938 .One can simply set the "Extras" field to:
{"source": "redash"}Related Tickets & Documents
This is an iteration of #3909. I've added type validation and help text.
Mobile & Desktop Screenshots/Recordings (if there are UI changes)