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

Support empty fields array in mappings #6133

Closed
edast opened this Issue May 12, 2014 · 15 comments

Comments

Projects
None yet
4 participants
@edast
Copy link

edast commented May 12, 2014

could you also change src/main/java/org/elasticsearch/index/mapper/core/TypeParsers.java (line 66) to support empty array for "fields"?

   } else if (fieldName.equals("fields")) {
       Map<String, Object> fieldsNode = (Map<String, Object>) fieldNode;

similar to #5887

@cfontes

This comment has been minimized.

Copy link
Contributor

cfontes commented Jul 23, 2014

I could try... Any details about it besides that statement?

@edast

This comment has been minimized.

Copy link
Author

edast commented Jul 23, 2014

not that i know of...

@clintongormley

This comment has been minimized.

Copy link
Member

clintongormley commented Jul 23, 2014

@cfontes see #5887 - it had the same issue

@edast

This comment has been minimized.

Copy link
Author

edast commented Jul 23, 2014

yes - I've reported that and it was solved - it is referenced in description

@clintongormley

This comment has been minimized.

Copy link
Member

clintongormley commented Jul 23, 2014

No, it's similar, not a duplicate.

@cfontes

This comment has been minimized.

Copy link
Contributor

cfontes commented Jul 23, 2014

Yeah I got that the second after the post was sent. So I deleted it.

I will give it a shot, see if I can come up with something.

@jpountz

This comment has been minimized.

Copy link
Contributor

jpountz commented Aug 1, 2014

@cfontes That would be great! Have you been able to make some progress? If you have any questions, feel free to ask!

@cfontes

This comment has been minimized.

Copy link
Contributor

cfontes commented Aug 1, 2014

Still haven't had time to do it.
But I plan to.
It's just going to take a while because I need to get more used to the code

@cfontes

This comment has been minimized.

Copy link
Contributor

cfontes commented Aug 5, 2014

In order to be on the safe side. I was thinking about creating a few unit tests.
I am a little confused about the best way to do that or if I am aiming at the right place to test this.

Which currently is

TypeParser.Mapper.TypeParser multiFieldConverterTypeParser

Any tips about what is the proper way on ElasticSearch to test this without having to use a real ParserContext (which has a massive constructor param list)? Mocking doens't look like an option as I couldn't find a framework for that on the deps.

@clintongormley

This comment has been minimized.

Copy link
Member

clintongormley commented Aug 5, 2014

@cfontes ++ for tests. Have a look at the test that was added to the related issue: https://github.com/elasticsearch/elasticsearch/pull/6006/files#diff-9a44deb15da4051f9980d8d6889d1684R56

@cfontes

This comment has been minimized.

Copy link
Contributor

cfontes commented Aug 6, 2014

Ok, cheers!

@cfontes

This comment has been minimized.

Copy link
Contributor

cfontes commented Aug 14, 2014

Sorry for the delay.

@clintongormley thanks for the tip, it helped a lot.

So I ran the whole test suite 2x (took 40 minutes each!) 2 tests failed, but one is a @badapple and the other one is marked as @LuceneTestCase.AwaitsFix

Tests with failures:

  • org.elasticsearch.indices.leaks.IndicesLeaksTests.testIndexShardLifecycleLeak
  • org.elasticsearch.discovery.DiscoveryWithNetworkFailuresTests.failWithMinimumMasterNodesConfigured

Is it ok to send the pull like that? I cannot run it again. It just takes to long and locks my machine.

Are they both @badapples or the second is my fault?

Cheers!

@clintongormley

This comment has been minimized.

Copy link
Member

clintongormley commented Aug 14, 2014

Hi @cfontes

I doubt either of those are your fault :)

I'd send the PR (mentioning this ticket number) and somebody will review it. thanks for the effort!

@cfontes

This comment has been minimized.

Copy link
Contributor

cfontes commented Aug 14, 2014

Nice,

Here it is #7271, if it needs any more work just ask.

Glad I could help, ES is great!

@jpountz jpountz removed adoptme labels Aug 27, 2014

@clintongormley

This comment has been minimized.

Copy link
Member

clintongormley commented Sep 6, 2014

Fixed by #7271

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.