Add metadata validation #527
Merged
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
amontanez24
reviewed
Nov 22, 2023
Comment on lines
+47
to
+56
| if 'tables' not in metadata: | ||
| raise ValueError( | ||
| 'Multi table reports expect metadata to contain a "tables" key with a mapping' | ||
| ' from table names to metadata for each table.' | ||
| ) | ||
| for table_name, table_metadata in metadata['tables'].items(): | ||
| if 'columns' not in table_metadata: | ||
| raise ValueError( | ||
| f'The metadata for table "{table_name}" is missing a "columns" key.' | ||
| ) |
There was a problem hiding this comment.
@npatki should we error in the case where the metadata is malformed? More specifically, f a users gives metadata with an empty table or empty column, should we error?
There was a problem hiding this comment.
@amontanez24 yeah that's probably a good idea. Though I'm neutral on whether or not there's an explicit error message for this case vs. the report just naturally crashing because it cannot access those key names.
npatki
approved these changes
Nov 22, 2023
Comment on lines
+47
to
+56
| if 'tables' not in metadata: | ||
| raise ValueError( | ||
| 'Multi table reports expect metadata to contain a "tables" key with a mapping' | ||
| ' from table names to metadata for each table.' | ||
| ) | ||
| for table_name, table_metadata in metadata['tables'].items(): | ||
| if 'columns' not in table_metadata: | ||
| raise ValueError( | ||
| f'The metadata for table "{table_name}" is missing a "columns" key.' | ||
| ) |
There was a problem hiding this comment.
@amontanez24 yeah that's probably a good idea. Though I'm neutral on whether or not there's an explicit error message for this case vs. the report just naturally crashing because it cannot access those key names.
R-Palazzo
added a commit
that referenced
this pull request
Nov 27, 2023
R-Palazzo
added a commit
that referenced
this pull request
Nov 27, 2023
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.
CU-86ayrkg1a
Resolve #526