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/migrate sql gen to macro #461

Merged
merged 7 commits into from
Oct 25, 2024

Conversation

aiss93
Copy link
Contributor

@aiss93 aiss93 commented Oct 9, 2024

resolves #457

Description

The following PR addresses the following points:

  • Migrates the PySpark code for the Iceberg file format at a macro level, making the impl.py file more readable.
  • Fixes the get_columns_in_relation function to work for both Iceberg and non-Iceberg tables without hard-coding the catalog name.
  • Fixes the get_location table function to work for both Iceberg and non-Iceberg tables on macOS and Windows. The old os.path.join function didn’t work well on Windows OS, as it built a path using '' instead of '/', leading to S3 path location errors.
  • Adds a helper function to retrieve the Iceberg catalog namespace from the profile.yaml file, allowing users to name their Iceberg catalog namespace as desired.
  • Adds merge_exclude_columns and incremental_predicates features.

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt-glue next" section.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@moomindani moomindani added the enable-functional-tests This label enable functional tests label Oct 18, 2024
@moomindani
Copy link
Collaborator

@aiss93 Apologizes I forgot to submit my comments last week. Submitted them now. Having test_iceberg is good idea, but I think we still need to make sure that we have enough coverage in the test cases. Plz refer to my comments.

@aiss93
Copy link
Contributor Author

aiss93 commented Oct 22, 2024

Hi @moomindani do you have any additional comments or can we launch the functional tests ?
Thank you for your time

@moomindani
Copy link
Collaborator

Hi @moomindani do you have any additional comments or can we launch the functional tests ? Thank you for your time

Thanks for making changes based on my comments. Apologizes for delay in review. We are still reviewing the changes. It may take some time as our bandwidth is very limited this month, but let us update you.

@aiss93
Copy link
Contributor Author

aiss93 commented Oct 22, 2024

Sure thank you !

@moomindani moomindani merged commit 0ea95c1 into aws-samples:main Oct 25, 2024
5 checks passed
@moomindani
Copy link
Collaborator

Thank you for your contribution! This patch brought huge value, we really appreciate your contribution.

```
In both cases, the underlying catalog would be the AWS Glue Catalog, unique in your AWS Account and Region, and you would be able to work with the exact same data. Also make sure that if you change the name of the Glue Catalog Alias, you change it in all the other `--conf` where it's used:
When using the default value, the following spark configruation should be added to enable iceberg.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

configuration

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops thanks I did not notice. Addressed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor enable-functional-tests This label enable functional tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add missing features such as merge_exclude_columns and incremental_predicates
4 participants