-
Notifications
You must be signed in to change notification settings - Fork 100
Adding templating for table options #123
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
base: main
Are you sure you want to change the base?
Adding templating for table options #123
Conversation
chi-yang-db
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got concerns in the table json change, let's discuss this and make it a separate improvement
| "description": "\nSchema evolution mode\nTable 4 - Schema Evolution Mode", | ||
| "order": 47, | ||
| "default": "addNewColumns", | ||
| "enum": ["addNewColumns", "rescue", "none"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm wouldn't this limit the maximum number of the tables to be 4 for each schema? And this complicates the implementation by naming table_x_format_config. It's hard to maintain the code when adding a 5th table, new supported format or new supported config
I think this need further discussion and merge in a separate PR. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to just 1 table after discussing
| # Check for default storage (recommended setting) | ||
| logger.info(f"Checking default storage configuration for catalog {catalog_name}") | ||
| try: | ||
| if not envmanager.has_default_storage(ws, catalog_name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you double check and test this? The method signature (specifically, the order of parameters) does not match what you defined above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
method has been fixed!
| "name": "{{.table_1_name}}", | ||
| "format": "{{.table_1_format}}", | ||
| "format_options": { | ||
| {{- if eq .table_1_format "csv"}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avro, parquet are supported. Check the comment above, let's discuss and make this a separate improvement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avro and parquet are included in the template
- Fixed initialization.py to pass catalog_name before workspace_client - Fixed debug_table_config.ipynb to retrieve and pass catalog_name parameter - Both calls now match the function signature: has_default_storage(catalog_name: str, workspace_client: WorkspaceClient = None) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Instead of requiring users to build their own tables.json file manually, we built up to 4 tables using the go template. The template will prompt users with configuration options for each table based on format defined by autoloader options