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

Code duplication and precision concerns with convertIndexWordToFloat/String() #67

Closed
niklas88 opened this issue Jun 26, 2018 · 4 comments
Assignees
Projects

Comments

@niklas88
Copy link
Member

The functions convertIndexWordToFloat() and convertIndexWordToFloatString() share a lot of code. Yet we can't just use std::to_string(convertIndexWordToFloat(indexWord)) because we also store integers in the "float" index words and the current method preserves precision because we directly go to a string representation. Maybe we can still exploit some common code?

One other thing I wonder is whether it might make sense to use double, for convertIndexWordToFloa(). Afaik the current methods are called float only because of the common name while working with strings their precision isn't actually limited to 32 bits.

@niklas88 niklas88 changed the title Code duplication and precision concerns with convertIndexWordToFloat() and convertIndexWordToFloatString() Code duplication and precision concerns with convertIndexWordToFloat/String() Jun 26, 2018
@floriankramer
Copy link
Member

According to https://www.w3.org/TR/xmlschema-2/#float the xsd:float datatype only has to support 32 bits of precision, while xsd:decimal theoretically has arbitrary precision. As we currently store floats and decimals the same way I'd second the suggestion of switching to doubles. When doing this we should probably also add xsd:double to the datatypes we accept.

@floriankramer
Copy link
Member

We could try to merge the beginnings of both functions into a single function that does some common parsing of the index word and then returns the mantissa, exponent and signs. I'm uncertain if that would be good for the codes readability though.
Alternatively we could also add a new type of index word for integers. The change would potentially reduce the precision of integers in old indices, as they would then be interpreted as floats, but it would also allow us to merge the two float parsing functions, and give us a cleaner solution for integers. If we add a version to the index we could also detect the change and print a warning, that the precision of integers is reduced.

@niklas88 niklas88 added this to To do in QLever Aug 31, 2018
@niklas88 niklas88 moved this from To do to In progress in QLever Aug 31, 2018
@floriankramer
Copy link
Member

@niklas88 was this fixed in #199 ?

@niklas88
Copy link
Member Author

niklas88 commented Apr 5, 2019

Yes

@niklas88 niklas88 closed this as completed Apr 5, 2019
QLever automation moved this from In progress to Done Apr 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
QLever
  
Done
Development

No branches or pull requests

2 participants