Remove legacy Iceberg MV separate storage table support #21370
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
findepi
force-pushed
the
findepi/iceberg-mv-hideon
branch
2 times, most recently
from
8896096 to
59d5700
Compare
findepi
force-pushed
the
findepi/iceberg-mv-hideon
branch
from
59d5700 to
3e0090a
Compare
findepi
force-pushed
the
findepi/iceberg-mv-hideon
branch
from
3e0090a to
8197f0f
Compare
|
|
||
| private final List<PropertyMetadata<?>> materializedViewProperties; | ||
|
|
||
| @Inject | ||
| public IcebergMaterializedViewProperties(IcebergConfig icebergConfig, IcebergTableProperties tableProperties) | ||
| public IcebergMaterializedViewProperties( | ||
| @EnableMaterializedViewSeparateStorageTable boolean enableMaterializedViewSeparateStorageTable, // for tests of legacy materialized views |
There was a problem hiding this comment.
Don’t add this. We don’t want special code just for testing. We’re removing this pattern from the code base.
There was a problem hiding this comment.
Agreed this shouldn't be here, and should eventually move to test code. I didn't see the urgency to do this within one PR. What do you think?
There was a problem hiding this comment.
I don’t think we should add more test-only annotations to production code. This is an anti-pattern and we don’t want people to see this and think it is ok.
We’re keeping legacy code to create these views, so it’s not clear how much this change is helping at this stage.
There was a problem hiding this comment.
I think I could save you some time on your next review round, if you gave me a hint how you prefer want to see this.
There was a problem hiding this comment.
Can we create the legacy view manually in the test code? Or maybe we start an old version of Trino in Testcontainers to write the legacy view?
|
What is the deprecation plan here? I see that we're just removing these configs, but they aren't deprecated today. What makes them legacy? How long do we plan to support reading these old tables? How do users convert from the legacy to the new way? Should we have a callable procedure to migrate? If this is deprecated and we plan to remove read support, in say six months, then we could simply deprecate this for now and remove the code entirely. Otherwise, we need a way to test this that doesn't require leaving the writing code in the production code path. |
|
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
findepi
added
the
stale-ignore
No description provided.