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 table parse bugs #27

Closed

Conversation

coderfender
Copy link

The current table + schema parse bug fails as per the issue raised here.
This PR aims to fix those bugs and help users use SQLAlchemy layer on drill more effectively.
Also , I have tested these changes on my local machine / EC2 with superset queries

@JohnOmernik
Copy link
Owner

So this work may be in conflict with the work @cgivre is doing to try to propagate the full schemas into super set with dots in it. It looks like the work is using extensions only, but with drill, you can select from a directory of csv, json, tsv, etc files and have it work, so relying only the extension may be problematic. @cgivre, what are your thoughts here?

@coderfender
Copy link
Author

@JohnOmernik , I have added test cases for the each of the cases you mentioned above.Also , I tested the following scenarios in my personal EC2 cluster

  1. Select * from <file_name.extension> ( Works)
  2. Select * from namespace.schema.folder (Works)
  3. Select * from namespace.schema.folder.file.extension (Works)
  4. Select * from table_name (Works)

Also , this change would be more on a query side to populate the data. Also , this change would fix the issue in SQL lab and chart building UIs.

@JohnOmernik , @cgivre , Let me know what you think . If you think this change will be conflicting, then I can go ahead make changes to better integrate the changes

@JohnOmernik
Copy link
Owner

So, I want @cgirve to look at things for sure.

The sadrill.py removal of
if "@@@" in table_name:
table_name = table_name.replace("@@@", ".")

I think would hurt what he's trying to do with better Superset integration. So let's wait for his comment.

Another test to add that I know works in Drill is

select * from namespace.schema.folder/*.extension

@cgivre
Copy link
Collaborator

cgivre commented May 24, 2019

@JohnOmernik
There was a recent PR in Superset (apache/superset#7453) which eliminates the need for the @@@ code, so we are safe to remove that from the dialect. I need to test the other features and will do so this weekend.

@coderfender
Copy link
Author

@JohnOmernik , I will try to test select * from namespace.schema.folder/*.extension and update here

Added unit tests to handle wildcard query use case
@coderfender
Copy link
Author

I added a test case for the query you mentioned @JohnOmernik .

@coderfender
Copy link
Author

Hello Guys, do you think I need to make further changes to the PR or is it okay to merge ?

@JohnOmernik
Copy link
Owner

I just merged @cgivre latest. Do we still have issues or are we safe now with the updates in Superset?

@cgivre
Copy link
Collaborator

cgivre commented Sep 3, 2019 via email

@coderfender
Copy link
Author

So you guys think that this PR is no longer needed ?

@cgivre
Copy link
Collaborator

cgivre commented Jan 15, 2020

Is this PR still needed? I just pushed a change that fixes view handling. IMHO, this dialect is pretty solid.

@coderfender
Copy link
Author

No I think we can safely close this PR now that the issue is fixed on the Superset side

@cgivre
Copy link
Collaborator

cgivre commented Feb 6, 2020

Do you want to close this PR? Not sure that it is relevant anymore.

@coderfender
Copy link
Author

Closing the PR. I guess its not needed anymore

@coderfender coderfender closed this Feb 6, 2020
@coderfender coderfender deleted the fix_table_parse_bugs branch February 6, 2020 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants