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

fix: limit max length edge case #68

Merged
merged 2 commits into from
Jul 6, 2023
Merged

fix: limit max length edge case #68

merged 2 commits into from
Jul 6, 2023

Conversation

pnadolny13
Copy link
Contributor

Closes #67

Comment on lines +237 to +242
def _conform_max_length(jsonschema_type):
"""Alter jsonschema representations to limit max length to Snowflake's VARCHAR length."""
max_length = jsonschema_type.get("maxLength")
if max_length and max_length > SNOWFLAKE_MAX_STRING_LENGTH:
jsonschema_type["maxLength"] = SNOWFLAKE_MAX_STRING_LENGTH
return jsonschema_type
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason you need to

  1. get the max length from the schema
  2. check if it's over the max allowed value
  3. update in the json schema
  4. get the max length again in to_sql_type

Could this be simplified to doing something like

@staticmethod
def get_max_length(jsonschema_type: dict) -> int:
    max_length = jsonschema_type.get("maxLength", self.MAX_COLUMN_SIZE)  # MAX_COLUMN_SIZE could be set on the connector class of each target
    return min(max_length, self.MAX_COLUMN_SIZE)

@staticmethod
def to_sql_type(jsonschema_type: dict) -> sqlalchemy.types.TypeEngine:
    ...
    maxlength = SnowflakeConnector.get_max_length(jsonschema_type)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edgarrmondragon yeah I did that originally but for this implementation we call the super to_sql_type which ends up overwriting the raw max length value by default. I'm not 100% sure I understand this whole custom type mapping logic here but the test fails unless we either overwrite the jsonschema type or after we call the super we do something like this to fix that length also.

target_type = SQLConnector.to_sql_type(jsonschema_type)
maxlength = SnowflakeConnector.get_max_length(jsonschema_type)
if hasattr(target_type, 'length'):
    target_type.length = maxlength

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, you're right. I missed the call to SQLConnector.to_sql_type. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

bug: Invalid character length
2 participants