Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

BigDecimal columns are not mapped well #16

Closed
emfanitek opened this Issue Feb 22, 2014 · 10 comments

Comments

Projects
None yet
2 participants

I have seen a related issue in the original elasticsearch plugin:

mstein#43

The BigDecimal columns are mapped as object, and then cause a problem when it looks like the other way around it fails when unmarshalling a BigDecimal field.

This does not seem to have been addressed yet, despite a pull request that suggests otherwise here:

elasticsearch/elasticsearch#2188

This is old old stuff (1 year) I can't believe that this has not been addressed yet.

I am totally willing to do whatever work needs to be done in the plugin to make it behave on our side, if it has indeed been fixed on the ES side.

Making Bigdecimal fields not searchable seems like a drastic workaround. Anyone has a suggestion?

Owner

noamt commented Feb 23, 2014

I'm not too sure about the issue, or if it has been fixed.
If you can create a test/project that reproduces the issue, or a pull request I'll be happy to take a look at it.
Otherwise, it'll take me a little more time to investigate.

Absolutely I'll do that.
On Feb 23, 2014 9:58 AM, "Noam Y. Tenne" notifications@github.com wrote:

I'm not too sure about the issue, or if it has been fixed.
If you can create a test/project that reproduces the issue, or a pull
request I'll be happy to take a look at it.
Otherwise, it'll take me a little more time to investigate.

Reply to this email directly or view it on GitHubhttps://github.com/noamt/elasticsearch-gorm-plugin/issues/16#issuecomment-35827111
.

Test app here:

https://github.com/emfanitek/elasticsearch-gorm-plugin/blob/master/test-apps/es-bigdecimal.zip

Didn't know how to attach a binary file.

The app fails while starting up with the error:

[elasticsearch[Proteus][bulk][T#5]] ERROR index.IndexRequestQueue - Failed bulk item: MapperParsingException[object mapping for [author] tried to parse as object, but got EOF, has a concrete value been provided to it?]

Owner

noamt commented Feb 28, 2014

Excellent, thanks much!

Replacing

static searchable =  true

with:

static searchable = {
    age converter:'long'
}

Allows mapping and indexing, but the converter is ignored due to this piece of code in ElasticSearchMappingFactory

            } else if (scpm.getConverter() != null) {
                // Use 'string' type for properties with custom converter.
                // Arrays are automatically resolved by ElasticSearch, so no worries.
                propType = 'string'

Why can't we use this instead?

            } else if (scpm.getConverter() != null) {
                // Arrays are automatically resolved by ElasticSearch, so no worries.
                String requestedConverter = scpm.getConverter()
                propType = (requestedConverter in SUPPORTED_FORMAT) ? requestedConverter : 'string'

@noamt noamt self-assigned this Mar 4, 2014

Owner

noamt commented Mar 4, 2014

BTW, isn't the 'string' converter preferable over 'long' for BigDecimal?
Doesn't conversion to long lose precision?

Well, that depends on what you wish to do with it. If you wish to use elastic search's ability to claculate totals and means, you need a numeric value. So string is no good in that case. Bigdecimal is multi purpose, can contain decimals or not, so we need to leave it to the application to map it to whatever primitive ES type necessary: string, double, or long.

noamt added a commit that referenced this issue Mar 5, 2014

#16 BigDecimal columns are not mapped well:
Big decimals are treated by ES as doubles by default, otherwise as a string.
When creating the mapping based on the searchable domain configuration, map big decimals as a double field; if a string is required, one can use a converter for it.
Owner

noamt commented Mar 5, 2014

I've deployed a new snapshot including a fix for the mapping, care to try it out before I release?
You can find it here

Great! I will, as soon as I have a moment!

On Wed, Mar 5, 2014 at 9:32 AM, Noam Y. Tenne notifications@github.comwrote:

I've deployed a new snapshot including a fix for the mapping, care to try
it out?
You can find it herehttp://noams.artifactoryonline.com/noams/grails-elasticsearch-plugin-snapshots/org/grails/plugins/elasticsearch-gorm/0.0.2.x-SNAPSHOT/

Reply to this email directly or view it on GitHubhttps://github.com/noamt/elasticsearch-gorm-plugin/issues/16#issuecomment-36720024
.

Owner

noamt commented Mar 24, 2014

Will be included in the next release

@noamt noamt closed this Mar 24, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment