-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
[Data Sources] Add: MemSQL query runner #1746
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Please check my comments.
redash/query_runner/memsql_ds.py
Outdated
} | ||
|
||
}, | ||
"required": ["host", "port"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a secret
field too (look at other data sources for example).
redash/query_runner/memsql_ds.py
Outdated
|
||
schema[table_name] = {'name': table_name, 'columns': columns} | ||
except Exception, e: | ||
raise sys.exc_info()[1], None, sys.exc_info()[2] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the Exception handling?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
redash/query_runner/memsql_ds.py
Outdated
columns.append({ | ||
'name': column, | ||
'friendly_name': column, | ||
'type': None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to use TYPE_STRING
instead of None
.
requirements.txt
Outdated
@@ -42,4 +42,6 @@ xlsxwriter==0.9.3 | |||
pystache==0.5.4 | |||
parsedatetime==2.1 | |||
cryptography==1.4 | |||
oauthlib==2.0.0 | |||
WTForms==2.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two aren't needed.
Thanks. :) |
[Data Sources] Add: MemSQL query runner
https://github.com/memsql/memsql-python/
meanwhile the column types aren't available.
pending on memsql/memsql-python#8