Feat!: Add new table_format property alongside storage_format #3175
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
erindru
commented
Sep 25, 2024
| "table_type": exp.Literal.string("iceberg") | ||
| }, | ||
| # it's painfully slow, but it works | ||
| table_format="iceberg", |
There was a problem hiding this comment.
Promoting it to a top-level property makes it explicit and consistent across adapters
erindru
commented
Sep 25, 2024
| # if we cant detect any indication of Iceberg, this is a Hive table | ||
| if table_properties and (table_type := table_properties.get("table_type", None)): |
There was a problem hiding this comment.
This became a lot simpler, we now dont need to try and parse the table type out of a physical_properties entry
erindru
commented
Sep 25, 2024
erindru
commented
Sep 25, 2024
| @@ -149,9 +149,9 @@ def test_create_table_iceberg(adapter: AthenaEngineAdapter) -> None: | |||
| name test_table, | |||
| kind FULL, | |||
| partitioned_by (colc, bucket(16, cola)), | |||
| table_format iceberg, | |||
| storage_format parquet, | |||
| physical_properties ( | |||
There was a problem hiding this comment.
^ this is what it looks like inside a model definition
erindru
force-pushed
the
erin/table-format
branch
2 times, most recently
from
52724ca
to
78e63b4
Compare
izeigerman
reviewed
Sep 25, 2024
| properties.append( | ||
| exp.Property(this="write.format.default", value=exp.Literal.string(storage_format)) | ||
| ) | ||
| else: |
izeigerman
reviewed
Sep 25, 2024
izeigerman
reviewed
Sep 25, 2024
izeigerman
reviewed
Sep 25, 2024
erindru
force-pushed
the
erin/table-format
branch
from
78e63b4
to
3d40027
Compare
erindru
commented
Sep 25, 2024
.circleci/continue_config.yml
Outdated
| branches: | ||
| only: | ||
| - main | ||
| #filters: |
There was a problem hiding this comment.
Reminder to self: Uncomment these prior to merging
izeigerman
approved these changes
Sep 25, 2024
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.
Context: #3154 (comment)
Currently, the
storage_formatproperty is a bit overloaded. It's defined as setting the on-disk file format (egparquet,orc) which makes sense but it's also been overloaded in various places to include table formats such asiceberg.This PR introduces a
table_formatproperty to clearly separate table formats from storage formats.The problem with the current setup of a single property is that it makes it difficult to describe concepts like "Iceberg + ORC", "Iceberg + Parquet", "Hive + Ion" etc.
Since
orcandparquetare already valid values forstorage_format,table_formatis a new property to hold the table format (eghiveoriceberg). To keep backwards compatibility, it's opt-in, so engine adapters need to explicitly take advantage of it.Implementing this is a precursor for compatibility with models created using dbt-athena which cleanly separates these concepts.
It also means that any users that may have gone "all in" on a format like Iceberg/ORC can use SQLMesh to create tables that are compatible with their existing downstream consumers. Currently, SQLMesh assumes Parquet when
storage_format=iceberg