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

Add fix for pyodbc+mssql #6621

Merged
merged 2 commits into from Jan 13, 2019
Merged

Conversation

chinhngt
Copy link
Contributor

@chinhngt chinhngt commented Jan 8, 2019

  • Handle Row object returned by pyodbc/msodbc
  • Add tests

@chinhngt
Copy link
Contributor Author

chinhngt commented Jan 8, 2019

@marcusianlevine @xrmx Can you help to take a look? This should fix several issues reported earlier like:

#3934
#4210
#4211
#3075

@xrmx
Copy link
Contributor

xrmx commented Jan 8, 2019

@chinhngt you missed to write what is the problem you are fixing. I suppose one between the tuple or the iterator breaks something else later in the chain. Or maybe both? I don't know the code but maybe it's the caller that must be fixed as an iterator may be a good idea to save memory.

@chinhngt
Copy link
Contributor Author

chinhngt commented Jan 8, 2019

@xrmx Currently when using Superset with pyodbc, the data pyodbc returns is in the form of [pyodbc.Row, pyodbc.Row, ...] instead of [(tuple), (tuple), ...] so SupersetDataFrame throws exception on wrong shape (it got 1 column instead of N columns). This change converts pyodbc.Row object to regular list before passing to SupersetDataFrame. I think BigQuery (BQEngineSpec in db_engine_specs.py) is doing similar thing :)

@xrmx
Copy link
Contributor

xrmx commented Jan 9, 2019

@chinhngt thanks for the explanation. So you have to prepare data that pandas can digest for a DataFrame.
It looks like pyodbc always use Row classes as it implements the DB API, wouldn't it better to poke at the connection uri instead of looking at the data?

@chinhngt
Copy link
Contributor Author

@xrmx I don't see sqla uri is available within db_engine_specs. Any hint?

@xrmx
Copy link
Contributor

xrmx commented Jan 10, 2019

@chinhngt no clue, anyway it if works for you it's fine for me.

@chinhngt
Copy link
Contributor Author

@xrmx Yes, it works for both Azure SQL and Azure Datawarehouse for me.

@mistercrunch mistercrunch merged commit 284a0cc into apache:master Jan 13, 2019
@bolkedebruin
Copy link
Contributor

@mistercrunch as this PR was not rebased against master the license check did not kick in on the PR. It does have a new file and therefore master now fails building. To solve this have a PR with a license header on an existing file ;-)

@chinhngt chinhngt deleted the fix_mssql_master branch January 15, 2019 02:11
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.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 🚢 0.34.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants