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

Patch to address column definitions. #1

Conversation

lafent
Copy link
Contributor

@lafent lafent commented Nov 1, 2019

In the v4.2.4 schema the following columns are listed as 'text' type columns.

. learning_outcome_dim.description
. module_dim.name

In the MySQL specific liquid files, the learning_outcome_dim table was failing
to be generated at all (due to a DDL syntax error). The module_dim.name
column was too short for some of our actual data and that was generating an
error. I went back to the schema.json and made adjustment to both file
(fixing the syntax error and adjusting the column spec).

I also applied similar changes to the MSSQL, Oracle, and PostgresQL variants.
I am unable to verify that these are syntactically correct, but they match
the similar constructs for other tables in their respective dialects.

In the v4.2.4 schema the following columns are listed as 'text' type columns.

.   learning_outcome_dim.description
.   module_dim.name

In the MySQL specific liquid files, the learning_outcome_dim table was failing
to be generated at all (due to a DDL syntax error).  The module_dim.name
column was too short for some of our actual data and that was generating an
error.  I went back to the `schema.json` and made adjustment to both file
(fixing the syntax error and adjusting the column spec).

I also applied similar changes to the MSSQL, Oracle, and PostgresQL variants.
I am unable to verify that these are syntactically correct, but they match
the similar constructs for other tables in their respective dialects.
@robert-carroll
Copy link
Collaborator

@lafent Thank you for the contribution and willingness to support all formats.

I just recently got a row with a 319 length for module_dim.name, have you seen anything larger than this? I try not to use MAX, LONGTEXT, and CLOB for anything that should typically have a limit (especially in the Canvas UI), and reserve those lengths for when fields contain HTML or discussion/posts/comments to reduce table space and keep things optimized. I recently made the change locally to set this value to 500, maybe 1000 is appropriate. Does that seem OK to you?

For learning_outcome_dim.description, I can agree changing to LONGTEXT for MYSQL, largest length I have here is 14796 so far.

@lafent
Copy link
Contributor Author

lafent commented Nov 7, 2019

I found the module_dim.name hiccup at 258 characters. I initially tweaked it to 512 just to get by but then I looked at the schema.json to see what Canvas showed for the column. I opted to go up to LONGTEXT since that was the definition, but I agree that it seems a little crazy.

I'm 100% on board with bumping the short limits up incrementally, but maybe a note in the docs to alert everyone that those limits are in there would be appropriate?

@robert-carroll
Copy link
Collaborator

schema.json isn't the best for us, it's generated from a Redshift sample and has lots of inaccuracies when compared to the real data. I'd like to continue with incremental adjustments to the limits as we see in the real world. Can you bump module_dim.name to 512 for each format in your PR and I will study up on merging.

I will add a note to the README about field length limits being subjectively observed.

After some discussions we are reducing the max size for the name column
to 512 characters for each RDBMS.
@lafent
Copy link
Contributor Author

lafent commented Nov 9, 2019

Okay, this patch should set the column length to 512 for all versions. It looks like github has picked it up in my branch, so hopefully it won't cause any grief.

@robert-carroll robert-carroll merged commit 4be57ab into ccsd:master Nov 13, 2019
@robert-carroll
Copy link
Collaborator

Thanks for the contribution @lafent

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

Successfully merging this pull request may close these issues.

None yet

2 participants