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

Do not index `_type` when there is at most one type. #24363

Merged
merged 4 commits into from May 4, 2017

Conversation

Projects
None yet
4 participants
@jpountz
Contributor

jpountz commented Apr 27, 2017

This change makes _type behave pretty much like _index when
index.mapping.single_type is true.

@nik9000

I left some comments, mostly me being confused.

core/src/main/java/org/elasticsearch/index/mapper/MapperService.java Outdated
}*/
boolean singleType = false;
}
//boolean singleType = false;

This comment has been minimized.

@nik9000

nik9000 Apr 28, 2017

Contributor

Leftover?

This comment has been minimized.

@jpountz

jpountz May 3, 2017

Contributor

I just wanted to make indices require one type at most by default for testing, I will merge this change after #24428.

This comment has been minimized.

@nik9000

nik9000 May 4, 2017

Contributor

+1

// means the index has a single type and the type field is implicit
Function<MapperService, String> typeFunction = mapperService -> {
Collection<String> types = mapperService.types();
if (types.size() > 1) {

This comment has been minimized.

@nik9000

nik9000 Apr 28, 2017

Contributor

Why not assert types.size() <= 1?

This comment has been minimized.

@jpountz

jpountz May 3, 2017

Contributor

I like explicit exceptions better when they do not have performance implications

@@ -88,29 +92,12 @@ public MetadataFieldMapper getDefault(MappedFieldType fieldType, ParserContext c
}
static final class TypeFieldType extends StringFieldType {
private boolean fielddata;

This comment has been minimized.

@nik9000

nik9000 Apr 28, 2017

Contributor

Do we have to deprecate fielddata in 5.x if we're going to do this? I mean, we certainly don't recommend using it in 5.x but maybe we didn't warn about it?

This comment has been minimized.

@nik9000

nik9000 Apr 28, 2017

Contributor

Also we won't be able to open indices that had types with fielddata in 6.0 then. All fine with me so long as we deprecate it in 5.x.

This comment has been minimized.

@nik9000

nik9000 Apr 28, 2017

Contributor

Or if we have already deprecated it and I didn't notice. That'd be even better.

This comment has been minimized.

@jpountz

jpountz May 3, 2017

Contributor

actually this is just dead code removal: this property was never read or set. There is no change wrt fielddata on the _type field.

core/src/main/java/org/elasticsearch/index/mapper/TypeFieldMapper.java Outdated
if (types.size() > 1) {
throw new AssertionError();
}
String type = types.isEmpty() ? "doc" : types.iterator().next();

This comment has been minimized.

@nik9000

nik9000 Apr 28, 2017

Contributor

Here we return "doc" if we don't have any types but in termsQuery we return match_none. Do we expect to get here when we don't have any types?

This comment has been minimized.

@jpountz

jpountz May 3, 2017

Contributor

Good point, we should not reach here if there are no types. I'll fix.

}
} else {
if (indexOptions() == IndexOptions.NONE) {
throw new AssertionError();

This comment has been minimized.

@nik9000

nik9000 Apr 28, 2017

Contributor

Why not assert indexOptions() != IndexOptions.NONE?

This comment has been minimized.

@nik9000

nik9000 Apr 28, 2017

Contributor

Is it possible with this PR to set index options on type? Maybe we should prevent that entirely? The frequency is always going to be 1 and the postings are going to be boring.

This comment has been minimized.

@jpountz

jpountz May 3, 2017

Contributor

no it is not possible. index options will be NONE if index.mapping.single_type is true and DOCS otherwise, but the user cannot enable/disable indexing or change index options via the mappings: the update would be rejected.

typesBytes[i] = new BytesRef(types[i]);
MappedFieldType ft = mapperService().fullName(TypeFieldMapper.NAME);
if (ft != null) {
// ft might be null if no documents have been indexed yet

This comment has been minimized.

@nik9000

nik9000 Apr 28, 2017

Contributor

Do we end up with an empty list of types but a non-null mapper if we create an index through the API but don't put any documents in it?

This comment has been minimized.

@jpountz

jpountz May 3, 2017

Contributor

you would have an empty list of types and no mappers at all (including metadata mappers)

@jpountz

This comment has been minimized.

Contributor

jpountz commented May 3, 2017

Thanks for having a look @nik9000, I just pushed another commit.

@nik9000

nik9000 approved these changes May 4, 2017

core/src/main/java/org/elasticsearch/index/mapper/MapperService.java Outdated
}*/
boolean singleType = false;
}
//boolean singleType = false;

This comment has been minimized.

@nik9000

nik9000 May 4, 2017

Contributor

+1

jpountz added some commits Apr 27, 2017

Do not index `_type` when there is at most one type.
This change makes `_type` behave pretty much like `_index` when
`index.mapping.single_type` is true.

@jpountz jpountz merged commit 977016b into elastic:master May 4, 2017

1 of 2 checks passed

elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details

@jpountz jpountz deleted the jpountz:fix/do_not_index_type branch May 4, 2017

jpountz added a commit that referenced this pull request May 4, 2017

Do not index `_type` when there is at most one type. (#24363)
This change makes `_type` behave pretty much like `_index` when
`index.mapping.single_type` is true.

@clintongormley clintongormley added v6.0.0-alpha1 and removed v6.0.0 labels May 9, 2017

debadair added a commit to debadair/elasticsearch that referenced this pull request May 16, 2017

Do not index `_type` when there is at most one type. (elastic#24363)
This change makes `_type` behave pretty much like `_index` when
`index.mapping.single_type` is true.
@pickypg

This comment has been minimized.

Member

pickypg commented May 18, 2017

This should probably be marked as a breaking change. A lot of people use _type even when they shouldn't -- when they're only using a single _type for example -- and this will break such searches right?

@jpountz

This comment has been minimized.

Contributor

jpountz commented May 19, 2017

I did my best to make this backward compatible, eg. a query on the _type field returns either a match_all or a query that does not match anything depending on whether the queried type is the single type that exists in the mappings. Do you have queries that broke after this change?

@pickypg

This comment has been minimized.

Member

pickypg commented May 19, 2017

I took it for a spin with the most common ones that I've seen:

Works

{
  "query": {
    "term": { "_type": "doc" }
  }
}

{
  "query": {
    "match": { "_type": "doc" }
  }
}

{
  "query": {
    "query_string": {
      "query": "_type:doc"
    }
  }
}

{
  "sort": [ "_type" ]
}

But then I became malicious, looking for ways to break it:

Does not work

{
  "query": {
    "prefix": {
      "_type": "doc"
    }
  }
}

It's a pretty bogus example, so I'm not so worried about it. Forget the breaking change listing.

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