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

feat: add quoting support #65

Closed
wants to merge 3 commits into from
Closed

feat: add quoting support #65

wants to merge 3 commits into from

Conversation

cstruct
Copy link

@cstruct cstruct commented Nov 25, 2022

Add quoting support both where Presto and Hive style is used.

From request by @nicor88

@cstruct cstruct mentioned this pull request Nov 25, 2022
@nicor88
Copy link
Member

nicor88 commented Nov 25, 2022

@cstruct could you fix the linting issues?

@cstruct
Copy link
Author

cstruct commented Nov 25, 2022

Yes, of course! I'll fix it tomorrow when I'm home again.

Add quoting support both where Presto and Hive style is used.
@cstruct
Copy link
Author

cstruct commented Nov 28, 2022

Sorry for the wait! Forgot to do it during the weekend :)

@nicor88
Copy link
Member

nicor88 commented Nov 29, 2022

@cstruct I tested this PR, and somehow this changes breaks our iceberg implementation.

Here the error log that I'm getting:

An error occurred (InvalidRequestException) when calling the StartQueryExecution operation: line 2:22: mismatched input '"lakehouse"' expecting

For what I can see the first part where it breaks is the drop staement.
e.g.

that should be rendered as drop table if existslakehouse.example_iceberg``

@cstruct
Copy link
Author

cstruct commented Nov 29, 2022

@nicor88 I'm not familiar with the iceberg implementation but I pushed a commit with a guess for a fix. Could you try again? If this doesn't work I'll have to create a test setup and do some proper debugging.

@nicor88
Copy link
Member

nicor88 commented Nov 29, 2022

@cstruct we can help you out, most of the code is in helpers_iceberg and in incremental (not sure if it's needed here, it might be needed too).

As you can see we have a custom drop for iceberg, as we don't use the general drop overwrite, the reason behind is that a drop table in iceberg lead to actual data deletion in S3, but this is not valid in the other cases.

@cstruct
Copy link
Author

cstruct commented Nov 29, 2022

@nicor88 alright it doesn't seem to involved. Unfortunately I've managed to bork my local python env, might have time to fix and test later today otherwise later this week. For the time being I pushed a commit that changes to Hive style where dropping and creating external tables in helpers_iceberg which I know are required. If you have the time and feel for it you can give it a try again otherwise I'll look into it later.

@cstruct
Copy link
Author

cstruct commented Nov 29, 2022

For those that are interested the reason this broke support for iceberg tables is that previously relation.quote_character was set to an empty string causing no quoting. This PR updates it to the Presto quote char " causing all identifiers to be quoted with it unless you explicitly call relation.render_hive() instead of implicit relation.__str__() or explicit relation.render().

@nicor88
Copy link
Member

nicor88 commented Dec 12, 2022

@cstruct any plan to move this forward?

@mattiamatrix
Copy link
Member

Please get the latest changes from main and update the PR title to align with conventional commits style

@cstruct
Copy link
Author

cstruct commented Dec 13, 2022

Yes, I do plan to revisit this PR when I have the time and energy to do so, I can't give any guarantees as to when that will happen though. If you feel that you do not want to wait for that, the Allow edits and access to secrets by maintainers is checked so any maintainer can pick up form where I left off.

@nicor88 nicor88 changed the title Add quoting support feat: add quoting support Dec 23, 2022
@BrechtDeVlieger
Copy link
Contributor

What is still to be done for this PR to be merged? Are the latest changes to helpers_iceberg tested @nicor88? I'm willing to continue the work on this if need be. Just not sure if the iceberg tests are still failing or not..

@nicor88
Copy link
Member

nicor88 commented Feb 17, 2023

@BrechtDeVlieger helper_iceberg doesn't exist anymore, this PR require a rebase with main and a fix for supporting iceberg. We don't have specific Integration tests to cover iceberg, you might want to use a specific model to test this branch with it.

@Jrmyy Jrmyy mentioned this pull request Feb 20, 2023
3 tasks
@Jrmyy Jrmyy closed this in #152 Feb 21, 2023
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.

None yet

5 participants