Skip to content

ID as strings and Parsing array items #46

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

Merged
merged 3 commits into from
Jan 6, 2021

Conversation

YaphetKG
Copy link
Contributor

@YaphetKG YaphetKG commented Jan 5, 2021

There were two issues when I had used bulk loader with enforce schema,

  • The column specified as ID needed to be an Integer, I tried computing the hash of the string I had tried to use that as an ID but I was getting errors from Struct suggesting the generated hash value was to long to convert.
  • The second item was when parsing columns specified as Type array , it will try to detect the types of each item in the array. In doing so there are cases where we have items as strings that are not necessarily arrays for instance array that looks like ['selenophosphoric acid','trihydroxidoselenidophosphorus','[P(OH)3Se]'] , so I tried to catch the exception and pass it down

Please if you have better solutions to my approach I'd be happy to adopt

catches exceptions when inferring arrays back falls to treating them as strings
Treats ID types as strings
@lgtm-com
Copy link

lgtm-com bot commented Jan 5, 2021

This pull request introduces 1 alert when merging 7db461c into 516fa56 - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'

handles "ERROR - 'RelationType' object has no attribute 'start_namespace' ", when using enforce schema with out namespaces.
@lgtm-com
Copy link

lgtm-com bot commented Jan 5, 2021

This pull request introduces 1 alert when merging 25493a4 into 516fa56 - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'

@codecov
Copy link

codecov bot commented Jan 6, 2021

Codecov Report

Merging #46 (516fa56) into master (516fa56) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #46   +/-   ##
=======================================
  Coverage   88.83%   88.83%           
=======================================
  Files           8        8           
  Lines         448      448           
=======================================
  Hits          398      398           
  Misses         50       50           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 516fa56...25493a4. Read the comment docs.

Copy link
Contributor

@jeffreylovitz jeffreylovitz left a comment

Choose a reason for hiding this comment

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

Hi @YaphetKG,

Thanks for the contributions! These look like sensible changes to me. Ultimately, the ID column should be able to specify its type, but until then it is more natural to default to strings rather than numerics.

@jeffreylovitz jeffreylovitz merged commit 4db84e0 into RedisGraph:master Jan 6, 2021
@YaphetKG
Copy link
Contributor Author

YaphetKG commented Jan 6, 2021

@jeffreylovitz Awesome thanks

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.

2 participants