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

[WIP] Arbitrary table properties #9523

Closed
wants to merge 2 commits into from

Conversation

dain
Copy link
Member

@dain dain commented Oct 6, 2021

Add support for arbitrary table properties in Hive connector.

trino:tpch> create table example (id bigint) with (extra_property_food = 'taco');
CREATE TABLE
trino:tpch> show create table example;
           Create Table           
----------------------------------
 CREATE TABLE hive.tpch.example ( 
    id bigint                     
 )                                
 WITH (                           
    extra_property_food = 'taco', 
    format = 'ORC'                
 )                                
(1 row)

This PR will need:

  • review of SPI changes
  • review of naming convention in Hive
  • better error handling in Hive changes
  • maybe better prevention of aliasing known Hive properties
  • a review of unmanaged Hive properties for security issues
  • a flag to diisable extra properties
  • tests
  • documentation

Resolves #955
Alternative to #9475

dain added 2 commits October 5, 2021 21:19
Table properties now do not have to be predeclared.  Instead table
properties are dynamically resolved during table creation and during
show create table.  For the table properties metadata table only known
properties are displayed.
@dain
Copy link
Member Author

dain commented Oct 6, 2021

After further thought, I'm not sure we need to implement Hive extra properties this way. The alternative implementation #9475 used a single map to contain all of the properties. The concern I had was that was how we would update a map like that. You would have to read the map, modify it and post it back, but that could result in losing or restoring properties changed during the update. What I didn't realize is we have no commands in Trino to modify a table property. We could use the simpler map style properties in #9475, and then when we add the syntax for setting a table property, we make it:

ALTER TABLE table_name SET  PROPERTY property_name[.field_name]*

I think this might be a better approach.

cc @martint and @electrum

@findepi
Copy link
Member

findepi commented Oct 6, 2021

extra_property_food = 'taco',

this is not the way to go. BTW we already discussed the syntax and decided to go with catch-all map in Hive connector only.

this new approach has two problems

  • requires name mangling for property names that are not valid Trino property names; without mangling, hive dotted convention cannot be used
  • leaves user with the impression that anything works, and every property is supported by Trino. Sub-ideal UX, but also more questions to answer ("i set this property, why did Trino do this")

@mosabua
Copy link
Member

mosabua commented Oct 28, 2022

👋 @dain and @findepi .. from the discussion in this PR I conclude that we can close this. Correct?

@lmove
Copy link

lmove commented Nov 18, 2022

Hi! Our tool BreakBot found that this pull request introduces 2 breaking changes and deprecations and they appear to impact 4 of your clients. You can find the full BreakBot report in our fork repository: report for PR#9523.

We hope this information is valuable to you, and apologize otherwise. If you're willing to help, we would kindly ask for your help to fill in a 5-minutes survey about the report. Your feedback will help us improve the tool and provide a better experience for users in the future!

@mosabua mosabua closed this Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Add support for Hive auto.purge property
4 participants