Skip to content
This repository has been archived by the owner on Feb 8, 2022. It is now read-only.

Added getNULL check on type check and convertNumberToText as a optional command line parameter #6

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jmaleonard
Copy link

No description provided.

@jmaleonard jmaleonard closed this Jul 11, 2018
@jmaleonard jmaleonard reopened this Jul 11, 2018
@jmaleonard jmaleonard changed the title Added getNULL check on type check Added getNULL check on type check and convertNumberToText as a optional command line parameter Jul 11, 2018
@jmaleonard
Copy link
Author

Added convertNumberToText or convert-numbers-to-text as a command line parameter that allows numbers to be stored as string in postgress. This is to address an issue where javascript numbers were stored as a string in DynamoDB. Also prevents the creation of an additional columns where they are not needed.

Eg. amount = 0. Will create column "amount" but amount = "0" will create another column "amount_numeric".

When starting podyn with convert-numbers-to-text all numeric fields will be treated as strings.

@jmaleonard
Copy link
Author

jmaleonard commented Jul 11, 2018

I also created docker image that will account for this new command line args

} else if (typedValue.getS() != null) {
return TableColumnType.text;
} else if (typedValue.getSS() != null) {
return TableColumnType.jsonb;
Copy link

Choose a reason for hiding this comment

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

PR is longer open, but if someone sometime will use it please think about:
In columnValueFromDynamoValue TableColumnType.text is used for StringSet (SS) value, but column type is jsonb. So podyn will add jsonb column, but try to write text inside. In our case this lead to empty values and some debugging time.

Copy link
Author

Choose a reason for hiding this comment

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

Want me to close this PR? This solved my issue

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

Successfully merging this pull request may close these issues.

2 participants