Ct 276/iceberg support patch #651
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Found a way to identify iceberg tables given that spark returns an error when trying to execute "SHOW TABLE EXTENDED..." See https://issues.apache.org/jira/browse/SPARK-33393 Instead of show table extended a "DESCRIBE EXTENDED" is performed to retrieve the provider information. This allows for identification of iceberg through an is_iceberg member variable. Allow for multiple join conditions to allow for mutliple columns to make a row distinct Use is_iceberg everywhere handling iceberg tables differs from other sources of data.
[CT-276] Apache Iceberg Support #294 The _schema variable was used for non-iceberg tables but was being overridden by work for iceberg v2 tables. I've made it so the iceberg condition will set _schema rather than blanket changing the schema for all providers.
On second look I wasn't happy with my name choices for macro name and method, hopefully what I have now makes more sense. [CT-276] Apache Iceberg Support #294
Upon further investigation this check is not needed since self.database will not be set.
[skip ci]
Fix incremental runs
Add Iceberg to the list
- upstream dbt changed breaking the use of ParsedSourceDefinition, using SourceDefinition appears to work instead - Added in change to include iceberg in adapters.sql: macro spark__alter_column_comment
tox on certain platforms will complain that /bin/bash is not allowed to be used. I'm allowing it to be used with this change.
Fixed conflicts: dbt/adapters/spark/impl.py dbt/adapters/spark/relation.py dbt/include/spark/macros/materializations/incremental/incremental.sql dbt/include/spark/macros/materializations/incremental/strategies.sql Updated tox.ini removing my own addition of allowlist_externals, main has it set now.
Cleanup based on comments
- Fixing up merge conflicts around Exception/Error types - Putting back accidental newline removal in tox.ini
- in impl.py changed:
List[agate.Row]
to
AttrDict
This fixed the pre-commit error after merging in changes from main.
I noticed that two Spark tests are failing, so tried to revert some of the changes that seemed related. They are now passing and also my Iceberg dbt project is still running fine
VersusFacit
commented
Feb 24, 2023
Fokko
approved these changes
Feb 24, 2023
There was a problem hiding this comment.
Thanks @VersusFacit for working on this. Looks even better now it has been refactored. Ran some tests locally, and it looks great!
VersusFacit
commented
Feb 24, 2023
VersusFacit
commented
Feb 24, 2023
|
I've run my original testcase with this change and it is working for me! Thanks so much for this @VersusFacit, I'll close my other PR. |
mikealfare
reviewed
Feb 28, 2023
VersusFacit
commented
Feb 28, 2023
VersusFacit
force-pushed
the
CT-276/iceberg-support-patch
branch
from
f878748 to
36f1d4c
Compare
mikealfare
approved these changes
Mar 2, 2023
VersusFacit
commented
Mar 2, 2023
| @@ -111,4 +110,4 @@ | |||
| amount: ["integer", "number"] | |||
| amount_usd: ["decimal", "number"] | |||
| created: ["timestamp", "string"] | |||
| """ | |||
| """.strip() | |||
There was a problem hiding this comment.
Like my mom used to say when she was bundling me up for school, "Remember to strip your whitespace on seeds, dear"
|
Thanks @dparent1 for creating the PR and @VersusFacit & @mikealfare for picking this up! Great to see this happening! |
This made my day when I turned my computer on this morning. Also @Fokko was very much instrumental in keeping this PR alive, thanks goes to you as well as the others. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
resolves #432
Description
Patches to dparent1's pr here.
I have addressed all comments that I could find open/unaddressed in their PR. I also rewrite the bulk of the
list_relations_without_cachingmethod.Me running some
runs against my dockerized iceberg instance.Checklist
changie newto create a changelog entry