Proposed changes to GlueTable in mojap-metadata

[lalithanagarur]

Actual Changes post Discussion
(Original proposed changes are in a comment below)

• Created new readme.md under glue_converter, and added a link in the central readme.md to this.

• Updated the metadata json schema with glue_table_properties and enforced string type validation.

• Added 2 private functions functions _update_table_parameters and _get_table_parameters.

• _update_table_parameters adds the key, value pairs for primary_key and glue_table_properties from the metadata to the parameters parameter in the table input. Will not add any key, value pairs for keys in glue_table_properties that are protected properties defined in _glue_table_properties_aws and/or other protected glue table properties such as primary_key.

• _get_table_parameters adds glue_table_properties and/or primary_key from the table properties in the Glue Data Catalog to the metadata if glue_table_properties are defined or update_primary_key is True. Set glue_table_properties to "*" to add glue_table_properties to the metadata with all available table properties from the Glue Data Catalog.

• Added _update_table_parameters to generate_from_meta in GlueTable to read the glue_table_properties from the metadata schema and populate this and/or primary_key under table properties in the Glue Data Catalog. Raises a warning if there are any protected properties in glue_table_properties, and will not write these properties to the Glue Data Catalog.

• Added _get_table_parameters to generate_to_meta in GlueTable to get the table properties from the Glue Data Catalog and populate this in the metadata schema. Option for user to specify the glue table properties to retrieve - set to None by default, or "*" to get all. These key, value pairs (including primary_key if explicitly defined or if "*") will be populated in glue_table_properties in the metadata schema. Added functionality to update primary_key in the metadata, default is set to False.

• Added _update_table_parameters to generate_from_meta in GlueConverter.

• Added tests to check these changes.

[SoumayaMauthoorMOJ]

You can check for the existence of "glue_table_properties" in the metadata using if "glue_table_properties" in my_dict.keys(). In the case of glue table, it will always have glue properties, it's just you might chose not to extract it. How about renaming the variable has_table_properties to something like get_table_properties or update_table_properties depending on which action you're doing?

[lalithanagarur]

No worries - I got that my_dict would need to be replaced with the corresponding relevant dict from my code. I'm just trying ot understand if there is somewhere specific in my code where you think this check with be better than something I'm currently using?

[lalithanagarur]

As discussed on our call, I've made some updates to the code and renamed the variables to update_table_properties and get_table_properties.

[tmpks]

Do we need update_table_properties and get_table_properties at all? In the case of get_table_properties I don't think there's a problem including some extra metadata even in situations where it won't be used so it seems redundant to me. Re update_table_properties, in my use case I'd like to toggle updating the properties via their inclusion or not in the metadata. Do we want an extra guardrail for setting table properties?

[tmpks]

For the record we (@lalithanagarur and @SoumayaMauthoorMOJ ) met and agreed that we should:

• Remove update_table_properties and get_table_properties
• Define a list of table properties which we won't allow to be set via mojap-metadata (the ones which Glue itself uses)
• Include a check to prevent setting those disallowed properties

[lalithanagarur]

• I have removed the update_table_properties and get_table_properties arguments. Instead, it just looks for a glue_table_properties key, checks these aren't protected attributes and then makes the relevant updates to the table parameters.
• I've added a list of the protected Glue table properties which I got from this page. These table properties cannot be overwritten.

[davidbridgwood]

I'm a big fan of this, think that once its implemented we can make use of it in a few different places!

Question - have you thought about how to handle different data-types?The properties stored in the glue data catalog are always strings but we are likely to need to pass them as/convert them to different or more structured types (ints/lists etc.).

We do this in generate_models.py by passing a dict of transformations. I think this approach is probably too prescriptive to be implemented in mojap-metadata but wonder if there is a way to optionally pass a json-schema from which we could infer any dtype transformation which need to happen.

[lalithanagarur]

Thanks David!

In terms of your question, I haven't thought about that. Boto3 has validation in place that checks that any user supplied properties (i.e. glue_table_properties in our case) are type dict[str,str] so there can't be any different data types when using generate_from_meta. I assume you're referring to handling different data types after generate_to_meta and having some sort of functionality to identify what the data types are? As you say, the approach you mentioned is too prescriptive especially since the aim is too keep mojap-metadata as 'agnostic' as possible but maybe something that identifies the dtype could be useful. Keen to have some more conversations on this though to see what ideas you have!

[davidbridgwood]

@lalithanagarur as discussed i've created a new issue for this (#139) with hopefully a bit of a clearer description and a proposal for a potential implementation. Happy to discuss more and help out with implementation when it comes to picking this up.

[lalithanagarur]

Thanks for discussing this with me and creating an issue @davidbridgwood! Agree that this would be a good future enhancement and something I will look into!

[lalithanagarur]

• @davidbridgwood I've made some changes to the code following on from other comments in the discussion. This also now means that you can provide glue_table_properties in types other than just strings, and in generate_to_meta the glue_table_properties are returned in more structured types too. Please let me know your thoughts on this.

[tmpks]

Should we also support setting glue_table_properties when using the GlueConverter class directly? I can find plenty of examples of code that uses GlueConverter rather than GlueTable (including the example in the README).

[tmpks]

As discussed (@lalithanagarur @SoumayaMauthoorMOJ @Thomas-Hirsch) we will support setting glue_table_properties when using the GlueConverter class directly.

[lalithanagarur]

I wanted to get your thoughts on the changes I've made to the methods in the GlueTable class first, and then I'll update the GlueConverter class accordingly.

[SoumayaMauthoorMOJ]

As discussed @lalithanagarur :

• get primary_key from

  mojap-metadata/mojap_metadata/metadata/specs/table_schema.json

  Line 354 in 8ebaa52

  "primary_key": {

• identify glue table attributes used by glue and make them protected so that we can't overwrite those values
• include primary_key in the list of protected glue table properties so that users only define primary_key in one location

[lalithanagarur]

• I've updated the code so it uses the primary_key provided in the above section of the metadata and not from glue_table_properties.
• I've added a list of the protected Glue table properties which I got from this page and also added primary_key to this list. These table properties cannot be overwritten by the values provided in glue_table_properties.

[SoumayaMauthoorMOJ]

About making get_table_properties and update_table_properties True by default:

• Makes sense for update_table_properties because if you're providing values then it makes sense to also update glue table properties
• Makes less sense for get_table_properties because it might not contain any user-defined values and then all you will extract is AWS-defined values (or nothing depending on the logic)

On a related note, is it possible to modify get_table_properties so you only get values you're interested in? cc @lalithanagarur @tmpks

[lalithanagarur]

• I tried to modify the code to only get the values the user is interested in by using **kwargs as an argument however this made the generate_to_meta code too complex. So I've updated the code to get any non protected glue table properties only.

[lalithanagarur]

In respect to Glue table properties that we don't want overwritten - I can't see anything in relation to Iceberg tables. I've found 'In accordance with Iceberg specifications, table properties are stored in the Iceberg table metadata file rather than in AWS Glue.'

So I'm going with the list that is here. If anyone has any other relevant links - please let me know. I can't seem to find a list of table parameters set in the Glue Catalog by any other services than Glue Crawler.

[lalithanagarur]

Update

• Added glue_table_properties_custom and glue_table_properties_aws to table_schema.json in mojap-metadata.

• The _custom prefix is to differentiate glue table properties set by users and glue table properties set by AWS.

• An error will be returned if glue_table_properties_custom is not a dictionary or if any values in glue_table_properties_custom are comma separated strings.

• glue_table_properties_aws will be populated with glue table properties set by AWS if the argument get_glue_table_properties_aws in generate_to_meta is set to True - the default value is False.

• Updated generate_from_meta and generate_to_meta and created _get_glue_table_properties method in GlueTable.

generate_from_meta will check if a primary_key and/or glue_table_properties_custom is populated in the table schema and then update table properties in the Glue Catalog with these key-value pairs. The values will be converted to strings e.g. 10 -> "10", ["value1", "value2"] -> "['value1', 'value2']", True -> "True".

_get_glue_table_properties loops through a dictionary containing table parameters and returns an updated metadata dictionary. If the primary_key is in the dictionary then it will add the primary_key to the metadata dictionary. It loops through the table parameters and separates them into custom and AWS glue properties and updates the metadata dictionary.

generate_to_meta updates the metadata dictionary with the primary_key and glue_table_properties_custom with the table parameters from the Glue Catalog. If include_glue_table_properties_aws (default is False) is set to True then the glue_table_properties_aws will also be updated in the metadata dictionary.

• Updated generate_from_meta in GlueConverter.
  generate_from_meta will check if a primary_key and/or glue_table_properties_custom is populated in the table schema and will update the metadata dictionary with these key-value pairs. The values will be converted to strings e.g. 10 -> "10", ["value1", "value2"] -> "['value1', 'value2']", True -> "True"

• Added tests to check these changes.

• Updated readme, changelog, and pyproject.

• Need to update https://github.com/moj-analytical-services/metadata_schema - PR. We should discuss if this separate repo is needed as the table schema is already in mojap-metadata?

[lalithanagarur]

Post our conversation on 22/05, we have agreed to:

1. Remove _custom and _aws prefix, just using glue_table_properties now.
2. Through metadata schema validation, enforcing that glue_table_properties must be a dictionary and the values must be of type string through metadata schema validation.
3. In generate_to_meta, user has to implicitly define the glue table properties to get or use "*" to get all glue table properties. Default value is an empty list []. Primary key is only populated in the metadata if the user has defined "primary_key" in glue_table_properties_to_get or is getting all glue table properties using"*".

• generate_from_meta will check if a primary_key and/or glue_table_properties is populated in the table schema and then update table properties in the Glue Catalog with these key-value pairs. The values in glue_table_properties in the table schema need to be type string.

• _get_glue_table_properties loops through the glue_table_properties_to_get list and gets the table parameters if they exist and returns an updated metadata dictionary. If primary_key is in glue_table_properties_to_get and it exists in the table parameters and evaluates to a list then it will update the primary_key value in the metadata dictionary. If "*" is in glue_table_properties_to_get then it will get all the available table parameters (including primary_key) and update the metadata dictionary accordingly.

• If glue_table_properties_to_get have been provided by the user, generate_to_meta updates the metadata dictionary with the primary_key and glue_table_properties with the specified table parameters from the Glue Catalog. If the user uses *, this will get all glue table properties from the Glue Catalog. The default value of glue_table_properties_to_get is an empty list [].

• Need to update v1.5.0 schema in https://github.com/moj-analytical-services/metadata_schema - updated v1.5.0 metadata_schema#13 to match these updates.

[lalithanagarur]

Changes to make:

• Change glue_table_properties_to_get to glue_table_properties and have the default as None in generate_to_meta.
• Change to "*", not ["*"] in generate_to_meta to get all the properties.
• Add warning if user has any protected glue table properties in glue_table_properties in the metadata scheme for generate_from_meta.
• Add argument in generate_to_meta and generate_from_meta where user can specify the primary key name i.e. primary_key_property = 'primary_key' is default, and can be user specified.
• Move the documentation into it's own section like for SQLAlchemy.

[lalithanagarur]

Add argument in generate_to_meta and generate_from_meta where user can specify the primary key name i.e. primary_key_property = 'primary_key' is default, and can be user specified.

Spoke to Soumaya about this, and this argument added a lot of unnecessary complexity so instead using the argument update_primary_key to update the primary_key in the metadata, default is set to False.

[lalithanagarur]

Original proposed Changes

• Curating tables in create-a-derived-table requires certain Glue catalog table properties to be set (these are just key/value pairs).
• We think the capability to set these should probably be added to the GlueTable class in mojap-metadata.
• These changes would support whatever Glue catalog table properties the user might like to set or get.
• The mojap-metadata format is specified in a JSON schema. The elements that are explicitly defined in the schema are enforced by the library, but users are free to add any other elements that suit their specific use cases (and this was an intentional decision when mojap-metadata was first developed)
• We still want to keep the mojap-metadata format generic and agnostic so we have only modified the generate_from_meta and generate_to_meta methods in the GlueTable class, and avoided making any changes to the mojap-metadata schema format.

The purpose of this discussion is to discuss the changes made to the methods in the GlueTable class and the name of the additional arguments. The PR for these changes is here.

• The additional argument added to the generate_from_meta and generate_to_meta methods is update_table_properties and get_table_properties respectively - the default value is set to False.
• If update_table_properties is set to True then the generate_from_meta method gets the glue_table_properties from the metadata and populates these key/value pairs in the Glue catalog.
• If get_table_properties is set to True, then the generate_to_meta method will get the table properties from the Glue catalog and populate this in the metadata.

As additional_properties and additional_parameters are used elsewhere in the repo, we thought that glue_table_properties would minimise confusion and promote explicit behaviour especially since table properties are Glue catalog specific.

We want to hear what thoughts and suggestions others may have here!